Johannes Sixt [off-list ref] writes:
quoted hunk
The deflate loop in bulk-checkin::stream_to_pack expects to get all bytes
from a file that it requests to read in a single function call. But it
used xread(), which does not give that guarantee. Replace it by
read_in_full().
Signed-off-by: Johannes Sixt <redacted>
---
The size is limited to sizeof(ibuf) == 16384 bytes, so that there
should not be a problem with the unpatched code on any OS in practice.
Nevertheless, this change seems reasonable from a code hygiene POV.
bulk-checkin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6b0b6d4..118c625 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
if (size && !s.avail_in) {
ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- if (xread(fd, ibuf, rsize) != rsize)
+ if (read_in_full(fd, ibuf, rsize) != rsize)
This is the kind of thing i was wondering and worried about with the
other "clipped xread/xwrite" patch. The original of this caller is
obviously wrong. Thanks for spotting and fixing.
I wonder if there are more like this broken caller or xread and/or
xwrite.
die("failed to read %d bytes from '%s'",
(int)rsize, path);
offset += rsize;
On Tue, Aug 20, 2013 at 5:00 PM, Junio C Hamano [off-list ref] wrote:
Johannes Sixt [off-list ref] writes:
quoted
The deflate loop in bulk-checkin::stream_to_pack expects to get all bytes
from a file that it requests to read in a single function call. But it
used xread(), which does not give that guarantee. Replace it by
read_in_full().
Signed-off-by: Johannes Sixt <redacted>
---
The size is limited to sizeof(ibuf) == 16384 bytes, so that there
should not be a problem with the unpatched code on any OS in practice.
Nevertheless, this change seems reasonable from a code hygiene POV.
bulk-checkin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6b0b6d4..118c625 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
if (size && !s.avail_in) {
ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
- if (xread(fd, ibuf, rsize) != rsize)
+ if (read_in_full(fd, ibuf, rsize) != rsize)
This is the kind of thing i was wondering and worried about with the
other "clipped xread/xwrite" patch. The original of this caller is
obviously wrong. Thanks for spotting and fixing.
I wonder if there are more like this broken caller or xread and/or
xwrite.
I was actually wondering when it's better to use xread() over
read_in_full() ? Considering that we don't know if xread() will read
the whole buffer or not, would it not be better to always use
read_in_full() ? I guess there is a drawback to this, but I'm not
exactly sure what it is.
Am 20.08.2013 17:00, schrieb Junio C Hamano:
I wonder if there are more like this broken caller or xread and/or
xwrite.
Looking at the grep -C1 output, there are no others.
The only one that looked suspicious was xread in remote-curl.c, but it is
fine (it just eats all data from the input).
-- Hannes
Am 20.08.2013 17:16, schrieb Antoine Pelisse:
I was actually wondering when it's better to use xread() over
read_in_full() ? Considering that we don't know if xread() will read
the whole buffer or not, would it not be better to always use
read_in_full() ?
Of course, you know whether the whole buffer was filled: xread() returns
the number of bytes read. Same for xwrite().
I guess there is a drawback to this, but I'm not
exactly sure what it is.
The disadvantage of read_in_full() is that it can happen that it reads
data from the stream successfully, but then reports an error. In this
case, the data read is lost.
Analogously, when write_in_full() writes (some) data successfully, but
ultimately fails, you don't know how much was written successfully.
-- Hannes