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

[René Scharfe]

Am 11.08.2018 um 18:48 schrieb Jeff King:

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.


I'm not sure, either.  It would catch media errors or file system
corruption, right?  Sounds useful, actually.  We should check the other
callers of strbuf_getline and friends as well, though, as reporting such
errors seems to be omitted in most cases:

$ git grep strbuf_getline | wc -l
99
$ git grep ferror | wc -l
35

René

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

[Jeff King]

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

> 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