Re: "git fsck" not detecting garbage at the end of blob object files...
On Sat, Jan 7, 2017 at 4:47 PM, Dennis Kaarsemaker wrote: > On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote: >> I was perusing StackOverflow this morning and ran across this >> question: >> http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/ >> >> It was a simple question about why "checking objects" was not >> appearing, but in it was another issue. The user purposefully >> corrupted a blob object file to see if `git fsck` would catch it by >> tacking extra data on at the end. `git fsck` happily said everything >> was okay, but when I played with things locally I found out that `git >> gc` does not like that extra garbage. I'm not sure what the trade-off >> needs to be here, but my expectation is that if `git fsck` says >> everything is okay, then all operations using that object (file) >> should work too. >> >> Is that unreasonable? What would be the impact of fixing this issue? > > If you do this with a commit object or tree object, fsck does complain. > I think it's sensible to do so for blob objects as well. Also very good information. Thanks Dennis! -John
Re: "git fsck" not detecting garbage at the end of blob object files...
On Sun, Jan 8, 2017 at 12:26 AM, Jeff King wrote: > On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote: >> On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote: >> > I was perusing StackOverflow this morning and ran across this >> > question: >> > http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/ >> > >> > It was a simple question about why "checking objects" was not >> > appearing, but in it was another issue. The user purposefully >> > corrupted a blob object file to see if `git fsck` would catch it by >> > tacking extra data on at the end. `git fsck` happily said everything >> > was okay, but when I played with things locally I found out that `git >> > gc` does not like that extra garbage. I'm not sure what the trade-off >> > needs to be here, but my expectation is that if `git fsck` says >> > everything is okay, then all operations using that object (file) >> > should work too. >> > >> > Is that unreasonable? What would be the impact of fixing this issue? >> >> If you do this with a commit object or tree object, fsck does complain. >> I think it's sensible to do so for blob objects as well. > > The existing extra-garbage check is in unpack_sha1_rest(), which is > called as part of read_sha1_file(). And that's what we hit for commits > and trees. However, we check the sha1 of blobs using the streaming > interface (in case they're large). I think you'd want to put a similar > check into read_istream_loose(). But note if you are grepping for it, it > is hidden behind a macro; look for read_method_decl(loose). That's for the pointer. > I'm actually not sure if this should be downgrade to a warning. It's > true that it's a form of corruption, but it doesn't actually prohibit us > from getting the data we need to complete the operation. Arguably fsck > should be more picky, but it is just relying on the same parse_object() > code path that the rest of git uses. > > I doubt anybody cares too much either way, though. It's not like this is > a common thing. I kind of wonder about that myself too, and I'm not sure what to think about it. On the one hand, I'd like to know about *anything* that has changed in an adverse way--it could indicate a failure somewhere else that needs to be handled. On the other hand, scaring the user isn't all that advantageous. I guess I'm in the former camp. As to whether this is common, yeah, it's probably not. However, I was surprised by the number of results that turned up when I search for "garbage at end of loose object". > I did notice another interesting case when looking at this. Fsck ends up > in fsck_loose(), which has the sha1 and path of the loose object. It > passes the sha1 to fsck_sha1(), and ignores the path entirely! > > So if you have a duplicate copy of the object in a pack, we'd actually > find and check the duplicate. This can happen, e.g., if you had a loose > object and fetched a thin-pack which made a copy of the loose object to > complete the pack). > > Probably fsck_loose() should be more picky about making sure we are > reading the data from the loose version we found. Interesting find! Thanks for the information Peff! -John
Re: "git fsck" not detecting garbage at the end of blob object files...
On Sat, Jan 07, 2017 at 10:47:03PM +0100, Dennis Kaarsemaker wrote: > On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote: > > I was perusing StackOverflow this morning and ran across this > > question: > > http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/ > > > > It was a simple question about why "checking objects" was not > > appearing, but in it was another issue. The user purposefully > > corrupted a blob object file to see if `git fsck` would catch it by > > tacking extra data on at the end. `git fsck` happily said everything > > was okay, but when I played with things locally I found out that `git > > gc` does not like that extra garbage. I'm not sure what the trade-off > > needs to be here, but my expectation is that if `git fsck` says > > everything is okay, then all operations using that object (file) > > should work too. > > > > Is that unreasonable? What would be the impact of fixing this issue? > > If you do this with a commit object or tree object, fsck does complain. > I think it's sensible to do so for blob objects as well. The existing extra-garbage check is in unpack_sha1_rest(), which is called as part of read_sha1_file(). And that's what we hit for commits and trees. However, we check the sha1 of blobs using the streaming interface (in case they're large). I think you'd want to put a similar check into read_istream_loose(). But note if you are grepping for it, it is hidden behind a macro; look for read_method_decl(loose). I'm actually not sure if this should be downgrade to a warning. It's true that it's a form of corruption, but it doesn't actually prohibit us from getting the data we need to complete the operation. Arguably fsck should be more picky, but it is just relying on the same parse_object() code path that the rest of git uses. I doubt anybody cares too much either way, though. It's not like this is a common thing. I did notice another interesting case when looking at this. Fsck ends up in fsck_loose(), which has the sha1 and path of the loose object. It passes the sha1 to fsck_sha1(), and ignores the path entirely! So if you have a duplicate copy of the object in a pack, we'd actually find and check the duplicate. This can happen, e.g., if you had a loose object and fetched a thin-pack which made a copy of the loose object to complete the pack). Probably fsck_loose() should be more picky about making sure we are reading the data from the loose version we found. -Peff
Re: "git fsck" not detecting garbage at the end of blob object files...
On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote: > I was perusing StackOverflow this morning and ran across this > question: > http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/ > > It was a simple question about why "checking objects" was not > appearing, but in it was another issue. The user purposefully > corrupted a blob object file to see if `git fsck` would catch it by > tacking extra data on at the end. `git fsck` happily said everything > was okay, but when I played with things locally I found out that `git > gc` does not like that extra garbage. I'm not sure what the trade-off > needs to be here, but my expectation is that if `git fsck` says > everything is okay, then all operations using that object (file) > should work too. > > Is that unreasonable? What would be the impact of fixing this issue? If you do this with a commit object or tree object, fsck does complain. I think it's sensible to do so for blob objects as well. Editing blob object: hurricane:/tmp/moo (master)$ hexer .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485 hurricane:/tmp/moo (master)$ git status On branch master nothing to commit, working tree clean hurricane:/tmp/moo (master)$ git fsck Checking object directories: 100% (256/256), done. hurricane:/tmp/moo (master)$ git gc Counting objects: 3, done. error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485' fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in .git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt error: failed to run repack Editing tree object: hurricane:/tmp/moo (master)$ hexer .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20 hurricane:/tmp/moo (master +)$ git status error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20' fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20' fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt hurricane:/tmp/moo (master +)$ git fsck error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20' fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20' fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in .git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt Editing commit object: hurricane:/tmp/moo (master)$ echo test >> .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0 hurricane:/tmp/moo (master +)$ git status error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0' fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0' fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt !(128) hurricane:/tmp/moo (master +)$ git fsck error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0' fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0' fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in .git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt D.
