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

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

[René Scharfe]

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.

Signed-off-by: Rene Scharfe 
---
 fsck.c | 23 ++-
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fsck.c b/fsck.c
index a0cee0be59..83f4562390 100644
--- a/fsck.c
+++ b/fsck.c
@@ -183,8 +183,9 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,
 static void init_skiplist(struct fsck_options *options, const char *path)
 {
static struct oid_array skiplist = OID_ARRAY_INIT;
-   int sorted, fd;
-   char buffer[GIT_MAX_HEXSZ + 1];
+   int sorted;
+   FILE *fp;
+   struct strbuf sb = STRBUF_INIT;
struct object_id oid;
 
 	if (options->skiplist)

@@ -194,25 +195,21 @@ static void init_skiplist(struct fsck_options *options, 
const char *path)
options->skiplist = 
}
 
-	fd = open(path, O_RDONLY);

-   if (fd < 0)
+   fp = fopen(path, "r");
+   if (!fp)
die("Could not open skip list: %s", path);
-   for (;;) {
+   while (!strbuf_getline(, fp)) {
const char *p;
-   int result = read_in_full(fd, buffer, sizeof(buffer));
-   if (result < 0)
-   die_errno("Could not read '%s'", path);
-   if (!result)
-   break;
-   if (parse_oid_hex(buffer, , ) || *p != '\n')
-   die("Invalid SHA-1: %s", buffer);
+   if (parse_oid_hex(sb.buf, , ) || *p != '\0')
+   die("Invalid SHA-1: %s", sb.buf);
oid_array_append(, );
if (sorted && skiplist.nr > 1 &&
oidcmp([skiplist.nr - 2],
   ) > 0)
sorted = 0;
}
-   close(fd);
+   fclose(fp);
+   strbuf_release();
 
 	if (sorted)

skiplist.sorted = 1;
--
2.18.0