{"thread":{"id":"9638","subject":"[PATCH] Don't segfault if we failed to inflate a packed delta","startedAt":"2007-08-25T07:26:04Z","lastAt":"2007-08-25T17:56:09Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"51495","messageId":"20070825072604.GA20155@spearce.org","threadId":"9638","inReplyTo":null,"subject":"[PATCH] Don't segfault if we failed to inflate a packed delta","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-08-25T07:26:04Z","receivedAt":"2007-08-25T07:26:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Under some types of packfile corruption the zlib stream holding the\ndata for a delta within a packfile may fail to inflate, due to say\na CRC failure within the compressed data itself.  When this occurs\nthe unpack_compressed_entry function will return NULL as a signal to\nthe caller that the data is not available.  Unfortunately we then\ntried to use that NULL as though it referenced a memory location\nwhere a delta was stored and tried to apply it to the delta base.\nLoading a byte from the NULL address typically causes a SIGSEGV.\n\ncate on #git noticed this failure in `git fsck --full` where the\ncall to verify_pack() first noticed that the packfile was corrupt\nby finding that the packfile's SHA-1 did not match the raw data of\nthe file.  After finding this fsck went ahead and tried to verify\nevery object within the packfile, even though the packfile was\nalready known to be bad.  If we are going to shovel bad data at\nthe delta unpacking code, we better handle it correctly.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n sha1_file.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b219d4d..9978a58 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1570,6 +1570,10 @@ static void *unpack_delta_entry(struct packed_git *p,\n \t\t    (uintmax_t)base_offset, p->pack_name);\n \n \tdelta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size);\n+\tif (!delta_data)\n+\t\tdie(\"failed to unpack compressed delta\"\n+\t\t    \" at %\"PRIuMAX\" from %s\",\n+\t\t    (uintmax_t)curpos, p->pack_name);\n \tresult = patch_delta(base, base_size,\n \t\t\t     delta_data, delta_size,\n \t\t\t     sizep);\n-- \n1.5.3.rc6.17.g1911\n"},{"id":"51500","messageId":"7v7ink6mpf.fsf@gitster.siamese.dyndns.org","threadId":"9638","inReplyTo":"20070825072604.GA20155@spearce.org","subject":"[PATCH 1/2] blame: check return value from read_sha1_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-25T08:30:04Z","receivedAt":"2007-08-25T08:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"(Trivial #1).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-blame.c |   11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 0519339..dc88a95 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -98,6 +98,10 @@ static char *fill_origin_blob(struct origin *o, mmfile_t *file)\n \t\tnum_read_blob++;\n \t\tfile->ptr = read_sha1_file(o->blob_sha1, &type,\n \t\t\t\t\t   (unsigned long *)(&(file->size)));\n+\t\tif (!file->ptr)\n+\t\t\tdie(\"Cannot read blob %s for path %s\",\n+\t\t\t    sha1_to_hex(o->blob_sha1),\n+\t\t\t    o->path);\n \t\to->file = *file;\n \t}\n \telse\n@@ -1384,6 +1388,9 @@ static void get_commit_info(struct commit *commit,\n \t\tunsigned long size;\n \t\tcommit->buffer =\n \t\t\tread_sha1_file(commit->object.sha1, &type, &size);\n+\t\tif (!commit->buffer)\n+\t\t\tdie(\"Cannot read commit %s\",\n+\t\t\t    sha1_to_hex(commit->object.sha1));\n \t}\n \tret->author = author_buf;\n \tget_ac_line(commit->buffer, \"\\nauthor \",\n@@ -2382,6 +2389,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \n \t\tsb.final_buf = read_sha1_file(o->blob_sha1, &type,\n \t\t\t\t\t      &sb.final_buf_size);\n+\t\tif (!sb.final_buf)\n+\t\t\tdie(\"Cannot read blob %s for path %s\",\n+\t\t\t    sha1_to_hex(o->blob_sha1),\n+\t\t\t    path);\n \t}\n \tnum_read_blob++;\n \tlno = prepare_lines(&sb);\n-- \n1.5.3.rc6.23.g0058\n"},{"id":"51499","messageId":"7v3ay86mos.fsf@gitster.siamese.dyndns.org","threadId":"9638","inReplyTo":"20070825072604.GA20155@spearce.org","subject":"[PATCH 2/2] pack-objects: check return value from read_sha1_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-25T08:30:27Z","receivedAt":"2007-08-25T08:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"(Trivial #2).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-pack-objects.c |    6 ++++++\n 1 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 77481df..9b3ef94 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1356,6 +1356,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t/* Load data if not already done */\n \tif (!trg->data) {\n \t\ttrg->data = read_sha1_file(trg_entry->idx.sha1, &type, &sz);\n+\t\tif (!trg->data)\n+\t\t\tdie(\"object %s cannot be read\",\n+\t\t\t    sha1_to_hex(trg_entry->idx.sha1));\n \t\tif (sz != trg_size)\n \t\t\tdie(\"object %s inconsistent object length (%lu vs %lu)\",\n \t\t\t    sha1_to_hex(trg_entry->idx.sha1), sz, trg_size);\n@@ -1363,6 +1366,9 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \t}\n \tif (!src->data) {\n \t\tsrc->data = read_sha1_file(src_entry->idx.sha1, &type, &sz);\n+\t\tif (!src->data)\n+\t\t\tdie(\"object %s cannot be read\",\n+\t\t\t    sha1_to_hex(src_entry->idx.sha1));\n \t\tif (sz != src_size)\n \t\t\tdie(\"object %s inconsistent object length (%lu vs %lu)\",\n \t\t\t    sha1_to_hex(src_entry->idx.sha1), sz, src_size);\n-- \n1.5.3.rc6.23.g0058\n"},{"id":"51512","messageId":"alpine.LFD.0.999.0708250815150.25853@woody.linux-foundation.org","threadId":"9638","inReplyTo":"20070825072604.GA20155@spearce.org","subject":"Re: [PATCH] Don't segfault if we failed to inflate a packed delta","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-25T15:19:17Z","receivedAt":"2007-08-25T15:19:17Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 25 Aug 2007, Shawn O. Pearce wrote:\n> \n> cate on #git noticed this failure in `git fsck --full` where the\n> call to verify_pack() first noticed that the packfile was corrupt\n> by finding that the packfile's SHA-1 did not match the raw data of\n> the file.  After finding this fsck went ahead and tried to verify\n> every object within the packfile, even though the packfile was\n> already known to be bad.  If we are going to shovel bad data at\n> the delta unpacking code, we better handle it correctly.\n\nHmm. We should actually make \"unpack_entry()\" return print an error and \nreturn NULL for these cases, rather than die, I think.\n\nMost of the callers seem to already check for NULL (not \"load_tree()\" in \nfast-import.c), but for something like fsck, while \"die()\" is obviously \nbetter than a SIGSEGV, we should probably continue and try to see what \nelse we find.\n\n(Although, to be honest, it might not matter. If your pack-file is corrupt \nenough for this to trigger, there's seldom anything interesting fsck will \ntell, so in practical terms this probably isn't a big deal).\n\n\t\tLinus\n"},{"id":"51517","messageId":"7v8x7z5why.fsf@gitster.siamese.dyndns.org","threadId":"9638","inReplyTo":"alpine.LFD.0.999.0708250815150.25853@woody.linux-foundation.org","subject":"Re: [PATCH] Don't segfault if we failed to inflate a packed delta","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-25T17:56:09Z","receivedAt":"2007-08-25T17:56:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Hmm. We should actually make \"unpack_entry()\" return print an error and \n> return NULL for these cases, rather than die, I think.\n>\n> Most of the callers seem to already check for NULL (not \"load_tree()\" in \n> fast-import.c), but for something like fsck, while \"die()\" is obviously \n> better than a SIGSEGV, we should probably continue and try to see what \n> else we find.\n>\n> (Although, to be honest, it might not matter. If your pack-file is corrupt \n> enough for this to trigger, there's seldom anything interesting fsck will \n> tell, so in practical terms this probably isn't a big deal).\n\nBut if the delta does not apply we already die().  Last night I\nand Shawn actually were wondering if it makes sense to have\nunpack_entry() die -- quite the opposite of being nicer to fsck.\n"}]}