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

[Randall S. Becker]

On January 11, 2018 1:31 AM Jeff King wrote"
> 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 
> ---
>  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) {

I'm sorry about the spaces. Still trying to get my mailer fixed so that I can 
get there directly from git.

Thanks for the approval and subsequent.
Cheers,
Randall

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

[Randall S. Becker]

On January 11, 2018 1:21 AM , Jeff King wrote:
> On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:
> > This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
> > BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> > was the only place outside of wrapper.c.
> 
> For my own curiosity, what is SSIZE_MAX on your platform? BUFFERSIZE is
> only 64k. Do you really have 16-bit size_t?
> 
> I wondered if you would also need to set MAX_IO_SIZE, but it looks like we
> default it to SSIZE_MAX.

size_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).

Cheers,
Randall

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

[Jeff King]

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 
---
 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

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

[Jeff King]

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

> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c.

For my own curiosity, what is SSIZE_MAX on your platform? BUFFERSIZE is
only 64k. Do you really have 16-bit size_t?

I wondered if you would also need to set MAX_IO_SIZE, but it looks like
we default it to SSIZE_MAX.

-Peff

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

[Randall S. Becker]

On January 11, 2018 12:40 AM, I wrote:
> Subject: [PATCH] Replaced read with xread in transport-helper.c to fix
> SSIZE_MAX overun in t5509
> 
> This fix was needed on HPE NonStop NSE where SSIZE_MAX is less than
> BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
> was the only place outside of wrapper.c.
> 
> Signed-off-by: Randall S. Becker 
> ---
>  transport-helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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);

This 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 

Cheers,
Randall