Ping?
Samuel Bronson <naesten@xxxxxxxxx> writes:
> [Hmm, nobody seems ot have commented on this analysis; maybe reposting
> it with a subject containing [BUG] will help?]
>
> Samuel Bronson <naesten@xxxxxxxxx> writes:
>
>> The following message is a courtesy copy of an article
>> that has been posted to gmane.comp.version-control.git as well.
>>
>> Oh, I forgot to provide any analysis of the problem. Oops.
>>
>> It may be just as well, though; I was tired enough that I might have
>> botched it in any case. So, have an analysis:
>>
>> While inflate errors are obviously NOT GOOD, and should perhaps be
>> instantly fatal for most commands, git fsck is something of a special
>> case because it is useful to have *it* report as many corrupt objects as
>> possible in one run.
>>
>> Unfortunately, this is not currently the case, as shown by the provided
>> testcase.
>>
>> The output for this testcase is:
>>
>> ,----
>> | checking known breakage:
>> | hash1=ffffffffffffffffffffffffffffffffffffffff &&
>> | hash2=fffffffffffffffffffffffffffffffffffffffe &&
>> | mkdir -p .git/objects/ff &&
>> | echo not-zlib >$(sha1_file $hash1) &&
>> | test_when_finished "remove_object $hash1" &&
>> | echo not-zlib >$(sha1_file $hash2) &&
>> | test_when_finished "remove_object $hash2" &&
>> |
>> | # Return value is not documented
>> | test_might_fail git fsck 2>out &&
>> | cat out && echo ====== &&
>> | grep "$hash1.*corrupt" out &&
>> | grep "$hash2.*corrupt" out
>> |
>> | error: inflate: data stream error (incorrect header check)
>> | error: unable to unpack ffffffffffffffffffffffffffffffffffffffff header
>> | error: inflate: data stream error (incorrect header check)
>> | fatal: loose object ffffffffffffffffffffffffffffffffffffffff (stored
>> | in .git/objects/ff/ffffffffffffffffffffffffffffffffffffff) is
>> | corrupt
>> | ======
>> | fatal: loose object ffffffffffffffffffffffffffffffffffffffff (stored
>> | in .git/objects/ff/ffffffffffffffffffffffffffffffffffffff) is
>> | corrupt
>> | not ok 5 - fsck survives inflate errors # TODO known breakage
>> `----
>>
>> If I flip it from expect_failure to expect_success and run the test with
>> -i, then go into the trash directory and run "gdb ../../git-fsck", I can
>> obtain this (thoroughly rehearsed & trimmed) gdb transcript:
>>
>> ,----
>> | % gdb ../../git-fsck
>> | GNU gdb (Debian 7.7.1-3) 7.7.1
>> ...
>> | Reading symbols from ../../git-fsck...done.
>> | (gdb) break error
>> | Breakpoint 1 at 0x813d24c: file usage.c, line 143.
>> | (gdb) break die
>> | Breakpoint 2 at 0x813d152: file usage.c, line 94.
>> | (gdb) run
>> | Starting program: /home/naesten/hacking/git/git-fsck
>> | [Thread debugging using libthread_db enabled]
>> | Using host libthread_db library
>> | "/lib/i386-linux-gnu/i686/cmov/libthread_db.so.1".
>> | Checking object directories: 100% (256/256), done.
>> |
>> | Breakpoint 1, error (err=0x8182f7a "inflate: %s (%s)") at usage.c:143
>> | 143 {
>> | (gdb) bt
>> | #0 error (err=0x8182f7a "inflate: %s (%s)") at usage.c:143
>> | #1 0x081452ff in git_inflate (strm=0xbfffe6b8, flush=0)
>> | at zlib.c:144
>> | #2 0x08125367 in unpack_sha1_header (stream=<optimized out>,
>> | map=<optimized out>, mapsize=<optimized out>,
>> | buffer=<optimized out>, bufsiz=<optimized out>)
>> | at sha1_file.c:1515
>> | #3 0x08125546 in sha1_loose_object_info (
>> | sha1=sha1@entry=0x82659d4 '\377' <repeats 20 times>,
>> | oi=oi@entry=0xbfffe788) at sha1_file.c:2528
>> | #4 0x08126b2d in sha1_object_info_extended (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, oi=0xbfffe788, flags=1)
>> | at sha1_file.c:2565
>> | #5 0x0812666f in sha1_object_info (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, sizep=0x0)
>> | at sha1_file.c:2601
>> | #6 0x080f6941 in parse_object (
>> | sha1=0x82659d4 '\377' <repeats 20 times>) at object.c:247
>> | #7 0x080758ac in fsck_sha1 (
>> | sha1=sha1@entry=0x82659d4 '\377' <repeats 20 times>)
>> | at builtin/fsck.c:333
>> ...
>> | (gdb) c
>> | Continuing.
>> | error: inflate: data stream error (incorrect header check)
>> |
>> | Breakpoint 1, error (err=0x817c525 "unable to unpack %s header")
>> | at usage.c:143
>> | 143 {
>> | (gdb) bt
>> | #0 error (err=0x817c525 "unable to unpack %s header") at usage.c:143
>> | #1 0x08125564 in sha1_loose_object_info (
>> | sha1=sha1@entry=0x82659d4 '\377' <repeats 20 times>,
>> | oi=oi@entry=0xbfffe788) at sha1_file.c:2529
>> | #2 0x08126b2d in sha1_object_info_extended (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, oi=0xbfffe788, flags=1)
>> | at sha1_file.c:2565
>> | #3 0x0812666f in sha1_object_info (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, sizep=0x0)
>> | at sha1_file.c:2601
>> | #4 0x080f6941 in parse_object (
>> | sha1=0x82659d4 '\377' <repeats 20 times>) at object.c:247
>> ...
>> | (gdb) frame 4
>> | #4 0x080f6941 in parse_object (
>> | sha1=0x82659d4 '\377' <repeats 20 times>) at object.c:247
>> | warning: Source file is more recent than executable.
>> | 247 sha1_object_info(sha1, NULL) == OBJ_BLOB)) { // <-- first error
>> | (gdb) down
>> | #3 0x0812666f in sha1_object_info (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, sizep=0x0)
>> | at sha1_file.c:2601
>> | 2601 if (sha1_object_info_extended(sha1, &oi, LOOKUP_REPLACE_OBJECT)
>> | < 0)
>> | (gdb) fin
>> | Run till exit from #3 0x0812666f in sha1_object_info (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, sizep=0x0)
>> | at sha1_file.c:2601
>> | error: unable to unpack ffffffffffffffffffffffffffffffffffffffff header
>> | parse_object (sha1=0x82659d4 '\377' <repeats 20 times>)
>> | at object.c:246
>> | 246 (!obj && has_sha1_file(sha1) &&
>> | Value returned is $1 = -1
>> | (gdb) c
>> | Continuing.
>> |
>> | Breakpoint 1, error (err=0x8182f7a "inflate: %s (%s)") at usage.c:143
>> | 143 {
>> | (gdb) bt
>> | #0 error (err=0x8182f7a "inflate: %s (%s)") at usage.c:143
>> | #1 0x081452ff in git_inflate (strm=0xbfffe710, flush=0)
>> | at zlib.c:144
>> | #2 0x08125367 in unpack_sha1_header (stream=<optimized out>,
>> | map=<optimized out>, mapsize=<optimized out>,
>> | buffer=<optimized out>, bufsiz=<optimized out>)
>> | at sha1_file.c:1515
>> | #3 0x08125429 in unpack_sha1_file (map=map@entry=0xb7fdc000,
>> | mapsize=<optimized out>, type=type@entry=0xbfffe7d8,
>> | size=0xbfffe7dc, sha1=0x82659d4 '\377' <repeats 20 times>)
>> | at sha1_file.c:1620
>> | #4 0x08126024 in read_object (
>> | sha1=sha1@entry=0x82659d4 '\377' <repeats 20 times>,
>> | type=type@entry=0xbfffe7d8, size=size@entry=0xbfffe7dc)
>> | at sha1_file.c:2667
>> | #5 0x08126c33 in read_sha1_file_extended (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, type=0xbfffe7d8,
>> | size=0xbfffe7dc, flag=1) at sha1_file.c:2690
>> | #6 0x080f69b8 in read_sha1_file (size=0xbfffe7dc, type=0xbfffe7d8,
>> | sha1=0x82659d4 '\377' <repeats 20 times>) at cache.h:853
>> | #7 parse_object (sha1=0x82659d4 '\377' <repeats 20 times>)
>> | at object.c:256
>> ...
>> | (gdb) c
>> | Continuing.
>> | error: inflate: data stream error (incorrect header check)
>> |
>> | Breakpoint 2, die (
>> | err=0x817cdbc "loose object %s (stored in %s) is corrupt")
>> | at usage.c:94
>> | 94 {
>> | (gdb) bt
>> | #0 die (err=0x817cdbc "loose object %s (stored in %s) is corrupt")
>> | at usage.c:94
>> | #1 0x08126cc1 in read_sha1_file_extended (
>> | sha1=0x82659d4 '\377' <repeats 20 times>, type=0xbfffe7d8,
>> | size=0xbfffe7dc, flag=1) at sha1_file.c:2705
>> | #2 0x080f69b8 in read_sha1_file (size=0xbfffe7dc, type=0xbfffe7d8,
>> | sha1=0x82659d4 '\377' <repeats 20 times>) at cache.h:853
>> | #3 parse_object (sha1=0x82659d4 '\377' <repeats 20 times>)
>> | at object.c:256
>> ...
>> | (gdb) frame 3
>> | #3 parse_object (sha1=0x82659d4 '\377' <repeats 20 times>)
>> | at object.c:256
>> | 256 buffer = read_sha1_file(sha1, &type, &size); // <-- death
>> | (gdb)
>> `----
>>
>> It's probably clearest what's happened here if I just show you this ...
>>
>> From object.c:
>>
>> struct object *parse_object(const unsigned char *sha1)
>> {
>> unsigned long size;
>> enum object_type type;
>> int eaten;
>> const unsigned char *repl = lookup_replace_object(sha1);
>> void *buffer;
>> struct object *obj;
>>
>> obj = lookup_object(sha1);
>> if (obj && obj->parsed)
>> return obj;
>>
>> if ((obj && obj->type == OBJ_BLOB) ||
>> (!obj && has_sha1_file(sha1) &&
>> sha1_object_info(sha1, NULL) == OBJ_BLOB)) { // <-- first error
>> if (check_sha1_signature(repl, NULL, 0, NULL) < 0) {
>> error("sha1 mismatch %s", sha1_to_hex(repl));
>> return NULL;
>> }
>> parse_blob_buffer(lookup_blob(sha1), NULL, 0);
>> return lookup_object(sha1);
>> }
>>
>> buffer = read_sha1_file(sha1, &type, &size); // <-- death
>> if (buffer) {
>> if (check_sha1_signature(repl, buffer, size, typename(type)) < 0) {
>> free(buffer);
>> error("sha1 mismatch %s", sha1_to_hex(repl));
>> return NULL;
>> }
>>
>> obj = parse_object_buffer(sha1, type, size, buffer, &eaten);
>> if (!eaten)
>> free(buffer);
>> return obj;
>> }
>> return NULL;
>> }
>>
>> (I've clearly added some marginal comments to my copy. ;-)
>>
>> The first two lines of output
>>
>>> error: inflate: data stream error (incorrect header check)
>>> error: unable to unpack ffffffffffffffffffffffffffffffffffffffff header
>>
>> come from deep within the call "sha1_object_info(sha1, NULL)".
>>
>> The next two lines
>>
>>> error: inflate: data stream error (incorrect header check)
>>> fatal: loose object ffffffffffffffffffffffffffffffffffffffff (stored
>>> in .git/objects/ff/ffffffffffffffffffffffffffffffffffffff) is
>>> corrupt
>>
>> and death come from the call "read_sha1_file(sha1, &type, &size)", which
>> is fair because that's exactly what the documentation comment above
>> read_sha1_file_extended() *says* will happen:
>>
>> /*
>> * This function dies on corrupt objects; the callers who want to
>> * deal with them should arrange to call read_object() and give error
>> * messages themselves.
>> */
>>
>> So, given that parse_object()'s documentation is:
>>
>> /*
>> * Returns the object, having parsed it to find out what it is.
>> *
>> * Returns NULL if the object is missing or corrupt.
>> */
>>
>> it probably should not call read_sha1_file() on a corrupt object.
>>
>> Options for fixing this would appear to include:
>>
>> 1. Saving the result of sha1_object_info(sha1, NULL) to a variable and
>> returning early if the object is corrupt. (But what happens if there
>> is corruption far enough in that it isn't seen when trying to grab
>> the object header?)
>>
>> 2. Calling read_object() and giving our own error messages.
>>
>> 3. Making read_sha1_file_extended only *optionally* die; since it's
>> calling die() directly.
>>
>> I'm also a bit nervous about this, though:
>>
>> static inline void *read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size)
>> {
>> return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
>> }
>>
>> Do we really want that happening while scanning the loose objects?
--
Hi! I'm a .signature virus! Copy me into your ~/.signature to help me spread!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html