Re: [PATCH] Replaced read with xread in transport-helper.c to fix SSIZE_MAX overun in t5509
- From
Jeff King <peff@peff.net>
- Date
- Jan 11, 2018, 06:31 UTC
- Message-ID
- <20180111063110.GB31213@sigill.intra.peff.net>
- In-Reply-To
- <010f01d38a9e$a5c4f290$f14ed7b0$@nexbridge.com>
On Thu, Jan 11, 2018 at 12:40:05AM -0500, Randall S. Becker wrote:
Show 13 quoted lines
> 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