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

Subsystems: the rest

STALE3131d

6 messages, 2 authors, 2018-01-11 · open the first message on its own page

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

From: Randall S. Becker <hidden>
Date: 2018-01-11 05:40:19

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 <redacted>
---
 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);
--

2.8.5.23.g6fa7ec3

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

From: Randall S. Becker <hidden>
Date: 2018-01-11 06:01:36

On January 11, 2018 12:40 AM, I wrote:

quoted hunk

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

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

From: Jeff King <hidden>
Date: 2018-01-11 06:20:56

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

From: Jeff King <hidden>
Date: 2018-01-11 06:31:18

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

quoted hunk

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

-- 
2.16.0.rc1.446.g4cb7d89c69

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

From: Randall S. Becker <hidden>
Date: 2018-01-11 13:40:23

On January 11, 2018 1:21 AM , Jeff King wrote:

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

quoted

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

From: Randall S. Becker <hidden>
Date: 2018-01-11 13:43:20

On January 11, 2018 1:31 AM Jeff King wrote"

quoted hunk

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

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help