git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
@ 2018-01-11  5:40 Randall S. Becker
  2018-01-11  6:01 ` Randall S. Becker
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Randall S. Becker @ 2018-01-11  5:40 UTC (permalink / raw)
  To: git mailing list

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 <rsbecker@nexbridge.com>
---
 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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
  2018-01-11  5:40 [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509 Randall S. Becker
@ 2018-01-11  6:01 ` Randall S. Becker
  2018-01-11  6:20 ` Jeff King
  2018-01-11  6:31 ` Jeff King
  2 siblings, 0 replies; 6+ messages in thread
From: Randall S. Becker @ 2018-01-11  6:01 UTC (permalink / raw)
  To: git mailing list

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 <rsbecker@nexbridge.com>
> ---
>  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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
  2018-01-11  5:40 [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509 Randall S. Becker
  2018-01-11  6:01 ` Randall S. Becker
@ 2018-01-11  6:20 ` Jeff King
  2018-01-11 13:40   ` Randall S. Becker
  2018-01-11  6:31 ` Jeff King
  2 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2018-01-11  6:20 UTC (permalink / raw)
  To: Randall S. Becker; +Cc: git mailing list

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
  2018-01-11  5:40 [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509 Randall S. Becker
  2018-01-11  6:01 ` Randall S. Becker
  2018-01-11  6:20 ` Jeff King
@ 2018-01-11  6:31 ` Jeff King
  2018-01-11 13:43   ` Randall S. Becker
  2 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2018-01-11  6:31 UTC (permalink / raw)
  To: Randall S. Becker; +Cc: git mailing list

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
  2018-01-11  6:20 ` Jeff King
@ 2018-01-11 13:40   ` Randall S. Becker
  0 siblings, 0 replies; 6+ messages in thread
From: Randall S. Becker @ 2018-01-11 13:40 UTC (permalink / raw)
  To: Jeff King; +Cc: git mailing list

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
  2018-01-11  6:31 ` Jeff King
@ 2018-01-11 13:43   ` Randall S. Becker
  0 siblings, 0 replies; 6+ messages in thread
From: Randall S. Becker @ 2018-01-11 13:43 UTC (permalink / raw)
  To: Jeff King; +Cc: git mailing list

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

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-11  5:40 [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509 Randall S. Becker
2018-01-11  6:01 ` Randall S. Becker
2018-01-11  6:20 ` Jeff King
2018-01-11 13:40   ` Randall S. Becker
2018-01-11  6:31 ` Jeff King
2018-01-11 13:43   ` Randall S. Becker

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox