Re: [PATCH 2/7] get-tar-commit-id: check write_in_full() return against 0
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Sep 13, 2017, 21:09 UTC
- Message-ID
- <20170913210927.GJ27425@aiede.mtv.corp.google.com>
- In-Reply-To
- <20170913171127.ocnitq6wz57ivhrh@sigill.intra.peff.net>
Hi,
Jeff King wrote:
Show 14 quoted lines
> We ask to write 41 bytes and make sure that the return value > is at least 41. This is the same "dangerous" pattern that > was fixed in the prior commit (wherein a negative return > value is promoted to unsigned), though it is not dangerous > here because our "41" is a constant, not an unsigned > variable. > > But we should convert it anyway to avoid modeling a > dangerous construct. > > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/get-tar-commit-id.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-)
I kind of disagree with calling this dangerous (and I think that is what you alluded to above by putting it in quotes), but I like the postimage more than the preimage.
The variable 'n' could be eliminated to simplify this further. I realize that would go against the spirit of this patch, but (1) it's on-topic for the patch, since it is another ssize_t vs constant comparison and (2) as mentioned elsewhere in this thread, it's a very common idiom with read_in_full. If we want to eliminate it then we could introduce a separate helper to distinguish between read_this_much_i_mean_it and read_this_much_or_to_eof.
Anyway, I think the below is an improvement.
With or without this tweak, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
diff --git i/builtin/get-tar-commit-id.c w/builtin/get-tar-commit-id.c index 6d9a79f9b3..03ef7c5ba4 100644 --- i/builtin/get-tar-commit-id.c +++ w/builtin/get-tar-commit-id.c @@ -20,13 +20,11 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix) struct ustar_header *header = (struct ustar_header *)buffer; char *content = buffer + RECORDSIZE; const char *comment; - ssize_t n; if (argc != 1) usage(builtin_get_tar_commit_id_usage); - n = read_in_full(0, buffer, HEADERSIZE); - if (n < HEADERSIZE) + if (read_in_full(0, buffer, HEADERSIZE) < HEADERSIZE) die("git get-tar-commit-id: read error"); if (header->typeflag[0] != 'g') return 1;