Re: [PATCH 1/2] fsck: use strbuf_getline() to read skiplist file

From: Jeff King <peff@peff.net>
Date: Aug 11, 2018, 16:48 UTC
Message-ID: <20180811164856.GB27393@sigill.intra.peff.net>
In-Reply-To: <c1ea6be5-57f7-68f1-0215-b4dceb07603a@web.de>

On Sat, Aug 11, 2018 at 05:39:27PM +0200, René Scharfe wrote:

Show 8 quoted lines

> The char array named "buffer" is unlikely to contain a NUL character, so
> printing its contents using %s in a die() format is unsafe.  Clang's
> ASan reports running over the end of buffer in the recently added
> skiplist tests in t5504-fetch-receive-strict.sh as a result.
> 
> Use an idiomatic strbuf_getline() loop instead, which ensures the buffer
> is always NUL-terminated.  As a side-effect this also adds support for
> skiplist files with CRLF line endings.

Nice. Two other side effects, both of which I think are good:

 - this is likely faster for a large skiplist due to fewer syscalls

 - this will no longer complain about a sha1 with a missing newline at
   the end-of-file (it will quietly handle it as if the newline were
   there)

And one I'm not sure about:

 - a read() error will now be quietly ignored; I guess we'd have to
   check ferror(fp) to cover this. I'm not sure if it matters.

>  fsck.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)

Code itself looks good to me (assuming we don't care about the read() error thing).

-Peff