RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509

On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:

quoted

diff --git a/transport-helper.c b/transport-helper.c index

3640804..68a4e30 100644

--- a/transport-helper.c
+++ b/transport-helper.c

@@ -1202,7 +1202,7 @@ static int udt_do_read(struct

unidirectional_transfer *t)

quoted

                return 0;       /* No space for more. */

        transfer_debug("%s is readable", t->src_name);
-       bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
+       bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE -
+ t->bufuse);
        if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
                errno != EINTR) {
                error_errno("read(%s) failed", t->src_name);

After this patch, I don't think we can ever see any of those errno values
again, as xread() will automatically retry in such a case.

I think that's OK. In the code before your patch, udt_do_read() would return
0 in such a case, giving the caller the opportunity to do something besides
simply retry the read. But the only caller is udt_copy_task_routine(), which
would just loop anyway.  It may be worth mentioning that in the commit
message.

So your patch is OK.  But we should probably clean up on top, like the patch
below (on top of yours; though note your patch was whitespace corrupted;
the tabs were converted to spaces).

-- >8 --
Subject: [PATCH] transport-helper: drop read/write errno checks

Since we use xread() and xwrite() here, EINTR, EAGAIN, and EWOULDBLOCK
retries are already handled for us, and we will never see these errno values
ourselves. We can drop these conditions entirely, making the code easier to
follow.

Signed-off-by: Jeff King <redacted>
---
 transport-helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c index

d48be722a5..fc49567ac4 100644

--- a/transport-helper.c
+++ b/transport-helper.c

@@ -1208,8 +1208,7 @@ static int udt_do_read(struct

unidirectional_transfer *t)

 	transfer_debug("%s is readable", t->src_name);
 	bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
-	if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
-		errno != EINTR) {
+	if (bytes < 0) {
 		error_errno("read(%s) failed", t->src_name);
 		return -1;
 	} else if (bytes == 0) {

@@ -1236,7 +1235,7 @@ static int udt_do_write(struct

unidirectional_transfer *t)

 	transfer_debug("%s is writable", t->dest_name);
 	bytes = xwrite(t->dest, t->buf, t->bufuse);
-	if (bytes < 0 && errno != EWOULDBLOCK) {
+	if (bytes < 0) {
 		error_errno("write(%s) failed", t->dest_name);
 		return -1;
 	} else if (bytes > 0) {