stream_to_pack: xread does not guarantee to read all requested bytes
PATCH 1/1 stream_to_pack: xread does not guarantee to read all requested bytes
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().
---
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(-)
bulk-checkin.c +1 -1
@@ -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)die ("failed to read %d bytes from '%s'" , (int )rsize, path); offset += rsize;
Signature · 1 line
1.8.4.rc3.1241.gcbfd92d.dirty
Quoted 19 lines · Johannes Sixt
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 byread_in_full().Signed-off-by Johannes Sixt <[email protected]>---
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(-)bulk-checkin.c +0
-0
bulk-checkin.c static int stream_to_pack(struct bulk_checkin_state *state, · line 117
Quoted 5 lines
bulk-checkin.c +1
-1@@ -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.
Quoted 3 lines
bulk-checkin.c +0
-0die ("failed to read %d bytes from '%s'" , (int )rsize, path); offset += rsize;
Quoted 31 lines · Junio C Hamano
Quoted 19 lines · Johannes Sixt
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 byread_in_full().Signed-off-by Johannes Sixt <[email protected]>---
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(-)bulk-checkin.c +0
-0bulk-checkin.c static int stream_to_pack(struct bulk_checkin_state *state, · line 117
Quoted 5 lines
bulk-checkin.c +1
-1@@ -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.
Quoted 2 lines
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:16, schrieb Antoine Pelisse:
Quoted 4 lines
I was actually wondering when it's better to use
xread()overread_in_full()? Considering that we don't know ifxread()will read the whole buffer or not, would it not be better to always useread_in_full()?
Of course, you know whether the whole buffer was filled: xread() returns the number of bytes read. Same for xwrite().
Quoted 2 lines
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
Quoted 2 lines · Antoine Pelisse
I was actually wondering when it's better to use
xread()overread_in_full()?
When the caller wants to do more control over a read that may have to loop. For example, this loop in builtin/index-pack.c::fill()
do {
ssize_t ret = xread(input_fd, input_buffer + input_len,
sizeof(input_buffer) - input_len);
if (ret <= 0) {
if (!ret)
die(_("early EOF"));
die_errno(_("read error on input"));
}
input_len += ret;
if (from_stdin)
display_throughput(progress, consumed_bytes + input_len);
} while (input_len < min);cannot be replaced blindly with read_in_full() because (1) the caller wants to do the "display_throughput()" part in the loop, and
(2) the caller wants to fill at least "min" bytes but can happily accept to read more up to the size of the input_buffer.
Am 20.08.2013 17:00, schrieb Junio C Hamano:
Quoted 2 lines
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
Quoted 2 lines · Junio C Hamano
I wonder if there are more like this broken caller or xread and/or xwrite.
Here is a result of a quick audit (of 1.8.0.x codebase).
As xwrite() will not be splitting a single-byte request, the patch to cat-file is more or less a theoretical fix, but if writing the date string can fail in I/O error, writing a terminating LF after it can fail the same way, so we should be consistent.
Everybody supports the side-band tranfer these days, so the patches to receive-pack and upload-pack are also theoretical fixes, I think. Note that in the more recent codebase, safe_write() is gone and we use write_or_die() instead in upload-pack.
builtin/cat-file.c | 2 +- builtin/receive-pack.c | 2 +- upload-pack.c | 5 ----- 3 files changed, 2 insertions(+), 7 deletions(-)
builtin/cat-file.c +1 -1
@@ -63,7 +63,7 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long tz =strtol (ep,NULL ,10 ); sp =show_date (date, tz,0 );write_or_die (1 , sp,strlen (sp)); -xwrite (1 ,"\n" ,1 ); +write_or_die (1 ,"\n" ,1 );break ; } }
builtin/receive-pack.c +1 -1
@@ -202,7 +202,7 @@ static void report_message(const char *prefix, const char *err, va_list params)if (use_sideband)send_sideband (1 ,2 , msg, sz, use_sideband);else -xwrite (2 , msg, sz); +write_in_full (2 , msg, sz); }static void rp_warning (const char *err, ...)
upload-pack.c +0 -5
@@ -64,11 +64,6 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)if (fd ==3 )/* emergency quit */ fd =2 ; -if (fd ==2 ) { -/* XXX: are we happy to lose stuff here? */ -xwrite (fd, data, sz); -return sz; - }return safe_write (fd, data, sz); }
Am 20.08.2013 20:52, schrieb Junio C Hamano:
Quoted 12 lines
Quoted 2 lines · Junio C Hamano
I wonder if there are more like this broken caller or xread and/or xwrite.
Here is a result of a quick audit (of 1.8.0.x codebase).
As
xwrite()will not be splitting a single-byte request, the patch to cat-file is more or less a theoretical fix, but if writing the date string can fail in I/O error, writing a terminating LF after it can fail the same way, so we should be consistent.
Quoted 4 lines
Everybody supports the side-band tranfer these days, so the patches to receive-pack and upload-pack are also theoretical fixes, I think. Note that in the more recent codebase,
safe_write()is gone and we usewrite_or_die()instead in upload-pack.
These changes look reasonable. Thank you!
Quoted 49 lines
builtin/cat-file.c | 2 +- builtin/receive-pack.c | 2 +- upload-pack.c | 5 ----- 3 files changed, 2 insertions(+), 7 deletions(-)builtin/cat-file.c +1
-1@@ -63,7 +63,7 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long tz =strtol (ep,NULL ,10 ); sp =show_date (date, tz,0 );write_or_die (1 , sp,strlen (sp)); -xwrite (1 ,"\n" ,1 ); +write_or_die (1 ,"\n" ,1 );break ; } }builtin/receive-pack.c +1
-1@@ -202,7 +202,7 @@ static void report_message(const char *prefix, const char *err, va_list params)if (use_sideband)send_sideband (1 ,2 , msg, sz, use_sideband);else -xwrite (2 , msg, sz); +write_in_full (2 , msg, sz); }static void rp_warning (const char *err, ...)upload-pack.c +0
-5@@ -64,11 +64,6 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)if (fd ==3 )/* emergency quit */ fd =2 ; -if (fd ==2 ) { -/* XXX: are we happy to lose stuff here? */ -xwrite (fd, data, sz); -return sz; - }return safe_write (fd, data, sz); }