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

[Jeff King <peff@peff.net>]

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

> 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)
>                 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 <peff@peff.net>
---
 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) {
-- 
2.16.0.rc1.446.g4cb7d89c69