git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Randall S. Becker" <rsbecker@nexbridge.com>
Cc: 'git mailing list' <git@vger.kernel.org>
Subject: Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
Date: Thu, 11 Jan 2018 01:31:10 -0500	[thread overview]
Message-ID: <20180111063110.GB31213@sigill.intra.peff.net> (raw)
In-Reply-To: <010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com>

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


  parent reply	other threads:[~2018-01-11  6:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-11  5:40 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 [this message]
2018-01-11 13:43   ` Randall S. Becker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180111063110.GB31213@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=rsbecker@nexbridge.com \
    --subject='Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).