{"thread":{"id":"47590","subject":"[PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","startedAt":"2018-01-11T05:40:19Z","lastAt":"2018-01-11T13:43:20Z","messageCount":6,"participants":["Randall S. Becker","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"336441","messageId":"010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com","threadId":"47590","inReplyTo":null,"subject":"[PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-11T05:40:05Z","receivedAt":"2018-01-11T05:40:19Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than\nBUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\nwas the only place outside of wrapper.c.\n\nSigned-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n---\n transport-helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 3640804..68a4e30 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1202,7 +1202,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n                return 0;       /* No space for more. */\n\n        transfer_debug(\"%s is readable\", t->src_name);\n-       bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n+       bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n        if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n                errno != EINTR) {\n                error_errno(\"read(%s) failed\", t->src_name);\n--\n2.8.5.23.g6fa7ec3\n\n\n"},{"id":"336442","messageId":"011001d38aa1$a097b160$e1c71420$@nexbridge.com","threadId":"47590","inReplyTo":"010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com","subject":"RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-11T06:01:25Z","receivedAt":"2018-01-11T06:01:36Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 11, 2018 12:40 AM, I wrote:\n> Subject: [PATCH] Replaced read with xread in transport-helper.c to fix\n> SSIZE_MAX overun in t5509\n> \n> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than\n> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\n> was the only place outside of wrapper.c.\n> \n> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>\n> ---\n>  transport-helper.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index 3640804..68a4e30 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1202,7 +1202,7 @@ static int udt_do_read(struct\n> unidirectional_transfer *t)\n>                 return 0;       /* No space for more. */\n> \n>         transfer_debug(\"%s is readable\", t->src_name);\n> -       bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> +       bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n>         if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n>                 errno != EINTR) {\n>                 error_errno(\"read(%s) failed\", t->src_name);\n\nThis fixes all known breaks except 3 on NonStop down from 229, so I'm thinking it's worth it. A high fix-to-bytes-changed ratio 😉\n\nCheers,\nRandall\n\n"},{"id":"336443","messageId":"20180111062048.GA31213@sigill.intra.peff.net","threadId":"47590","inReplyTo":"010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com","subject":"Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-11T06:20:49Z","receivedAt":"2018-01-11T06:20:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:\n\n> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than\n> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\n> was the only place outside of wrapper.c.\n\nFor my own curiosity, what is SSIZE_MAX on your platform? BUFFERSIZE is\nonly 64k. Do you really have 16-bit size_t?\n\nI wondered if you would also need to set MAX_IO_SIZE, but it looks like\nwe default it to SSIZE_MAX.\n\n-Peff\n"},{"id":"336444","messageId":"20180111063110.GB31213@sigill.intra.peff.net","threadId":"47590","inReplyTo":"010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com","subject":"Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-11T06:31:10Z","receivedAt":"2018-01-11T06:31:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 3640804..68a4e30 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1202,7 +1202,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n>                 return 0;       /* No space for more. */\n> \n>         transfer_debug(\"%s is readable\", t->src_name);\n> -       bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> +       bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n>         if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n>                 errno != EINTR) {\n>                 error_errno(\"read(%s) failed\", t->src_name);\n\nAfter this patch, I don't think we can ever see any of those errno\nvalues again, as xread() will automatically retry in such a case.\n\nI think that's OK. In the code before your patch, udt_do_read() would\nreturn 0 in such a case, giving the caller the opportunity to do\nsomething besides simply retry the read. But the only caller is\nudt_copy_task_routine(), which would just loop anyway.  It may be worth\nmentioning that in the commit message.\n\nSo your patch is OK.  But we should probably clean up on top, like the\npatch below (on top of yours; though note your patch was whitespace\ncorrupted; the tabs were converted to spaces).\n\n-- >8 --\nSubject: [PATCH] transport-helper: drop read/write errno checks\n\nSince we use xread() and xwrite() here, EINTR, EAGAIN, and\nEWOULDBLOCK retries are already handled for us, and we will\nnever see these errno values ourselves. We can drop these\nconditions entirely, making the code easier to follow.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex d48be722a5..fc49567ac4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1208,8 +1208,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n \n \ttransfer_debug(\"%s is readable\", t->src_name);\n \tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n-\tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n-\t\terrno != EINTR) {\n+\tif (bytes < 0) {\n \t\terror_errno(\"read(%s) failed\", t->src_name);\n \t\treturn -1;\n \t} else if (bytes == 0) {\n@@ -1236,7 +1235,7 @@ static int udt_do_write(struct unidirectional_transfer *t)\n \n \ttransfer_debug(\"%s is writable\", t->dest_name);\n \tbytes = xwrite(t->dest, t->buf, t->bufuse);\n-\tif (bytes < 0 && errno != EWOULDBLOCK) {\n+\tif (bytes < 0) {\n \t\terror_errno(\"write(%s) failed\", t->dest_name);\n \t\treturn -1;\n \t} else if (bytes > 0) {\n-- \n2.16.0.rc1.446.g4cb7d89c69\n\n"},{"id":"336458","messageId":"002101d38ae1$b1bacb40$153061c0$@nexbridge.com","threadId":"47590","inReplyTo":"20180111062048.GA31213@sigill.intra.peff.net","subject":"RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-11T13:40:01Z","receivedAt":"2018-01-11T13:40:23Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 11, 2018 1:21 AM , Jeff King wrote:\n> On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:\n> > This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than\n> > BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c\n> > was the only place outside of wrapper.c.\n> \n> For my own curiosity, what is SSIZE_MAX on your platform? BUFFERSIZE is\n> only 64k. Do you really have 16-bit size_t?\n> \n> I wondered if you would also need to set MAX_IO_SIZE, but it looks like we\n> default it to SSIZE_MAX.\n\nsize_t is 32 or 64 depending on the memory model of how a program is compiled. SSIZE_MAX in limits.h is 53284, which is a message system limit. There was a previous fix associated with this size limit came from our team (commit a983e6ac58094a3b2466ad3be13049ce213f9fc3).\n\nCheers,\nRandall\n\n"},{"id":"336459","messageId":"002201d38ae2$1f9b26a0$5ed173e0$@nexbridge.com","threadId":"47590","inReplyTo":"20180111063110.GB31213@sigill.intra.peff.net","subject":"RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-11T13:43:06Z","receivedAt":"2018-01-11T13:43:20Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 11, 2018 1:31 AM Jeff King wrote\"\n> On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:\n> > diff --git a/transport-helper.c b/transport-helper.c index\n> > 3640804..68a4e30 100644\n> > --- a/transport-helper.c\n> > +++ b/transport-helper.c\n> > @@ -1202,7 +1202,7 @@ static int udt_do_read(struct\n> unidirectional_transfer *t)\n> >                 return 0;       /* No space for more. */\n> >\n> >         transfer_debug(\"%s is readable\", t->src_name);\n> > -       bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> > +       bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE -\n> > + t->bufuse);\n> >         if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n> >                 errno != EINTR) {\n> >                 error_errno(\"read(%s) failed\", t->src_name);\n> \n> After this patch, I don't think we can ever see any of those errno values\n> again, as xread() will automatically retry in such a case.\n> \n> I think that's OK. In the code before your patch, udt_do_read() would return\n> 0 in such a case, giving the caller the opportunity to do something besides\n> simply retry the read. But the only caller is udt_copy_task_routine(), which\n> would just loop anyway.  It may be worth mentioning that in the commit\n> message.\n> \n> So your patch is OK.  But we should probably clean up on top, like the patch\n> below (on top of yours; though note your patch was whitespace corrupted;\n> the tabs were converted to spaces).\n> \n> -- >8 --\n> Subject: [PATCH] transport-helper: drop read/write errno checks\n> \n> Since we use xread() and xwrite() here, EINTR, EAGAIN, and EWOULDBLOCK\n> retries are already handled for us, and we will never see these errno values\n> ourselves. We can drop these conditions entirely, making the code easier to\n> follow.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  transport-helper.c | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/transport-helper.c b/transport-helper.c index\n> d48be722a5..fc49567ac4 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -1208,8 +1208,7 @@ static int udt_do_read(struct\n> unidirectional_transfer *t)\n> \n>  \ttransfer_debug(\"%s is readable\", t->src_name);\n>  \tbytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);\n> -\tif (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&\n> -\t\terrno != EINTR) {\n> +\tif (bytes < 0) {\n>  \t\terror_errno(\"read(%s) failed\", t->src_name);\n>  \t\treturn -1;\n>  \t} else if (bytes == 0) {\n> @@ -1236,7 +1235,7 @@ static int udt_do_write(struct\n> unidirectional_transfer *t)\n> \n>  \ttransfer_debug(\"%s is writable\", t->dest_name);\n>  \tbytes = xwrite(t->dest, t->buf, t->bufuse);\n> -\tif (bytes < 0 && errno != EWOULDBLOCK) {\n> +\tif (bytes < 0) {\n>  \t\terror_errno(\"write(%s) failed\", t->dest_name);\n>  \t\treturn -1;\n>  \t} else if (bytes > 0) {\n\nI'm sorry about the spaces. Still trying to get my mailer fixed so that I can get there directly from git.\n\nThanks for the approval and subsequent.\nCheers,\nRandall\n\n"}]}