{"thread":{"id":"60848","subject":"[PATCH] Always check the return value of `repo_read_object_file()`","startedAt":"2024-02-05T14:35:57Z","lastAt":"2024-02-18T22:36:37Z","messageCount":16,"participants":["Johannes Schindelin via GitGitGadget","Karthik Nayak","Kyle Lippincott","Patrick Steinhardt","Junio C Hamano","Johannes Schindelin","Teng Long"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487929","messageId":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","threadId":"60848","inReplyTo":null,"subject":"[PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-05T14:35:53Z","receivedAt":"2024-02-05T14:35:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThere are a couple of places in Git's source code where the return value\nis not checked. As a consequence, they are susceptible to segmentation\nfaults.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    Always check the return value of repo_read_object_file()\n    \n    I ran into this today, when I had tried git am -3 to import changes from\n    a repository into a different repository that has the first repository's\n    code vendored in. To make this work, I set\n    GIT_ALTERNATE_OBJECT_DIRECTORIES accordingly for the git am -3 call, but\n    forgot to set it for a subsequent git diff call, which then segfaulted.\n    \n    There are still a couple of places left where there are checks but they\n    look dubious to me, as they simply continue as if an empty blob had been\n    read, for example in builtin/tag.c. However, there are checks that avoid\n    segfaults, so I left them alone.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1650%2Fdscho%2Fsafer-object-reads-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1650/dscho/safer-object-reads-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1650\n\n bisect.c           |  3 +++\n builtin/cat-file.c | 10 ++++++++--\n builtin/grep.c     |  2 ++\n builtin/notes.c    |  6 ++++--\n combine-diff.c     |  2 ++\n rerere.c           |  3 +++\n 6 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex f1273c787d9..f75e50c3397 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,\n \t\tconst char *subject_start;\n \t\tint subject_len;\n \n+\t\tif (!buf)\n+\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&commit->object.oid));\n+\n \t\tfprintf(stderr, \"%c%c%c \",\n \t\t\t(commit_flags & TREESAME) ? ' ' : 'T',\n \t\t\t(commit_flags & UNINTERESTING) ? 'U' : ' ',\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 7d4899348a3..bbf851138ec 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -221,6 +221,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\t\t\t\t\t\t     &type,\n \t\t\t\t\t\t\t\t     &size);\n \t\t\t\tconst char *target;\n+\n+\t\t\t\tif (!buffer)\n+\t\t\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&oid));\n+\n \t\t\t\tif (!skip_prefix(buffer, \"object \", &target) ||\n \t\t\t\t    get_oid_hex(target, &blob_oid))\n \t\t\t\t\tdie(\"%s not a valid tag\", oid_to_hex(&oid));\n@@ -416,6 +420,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \n \t\tcontents = repo_read_object_file(the_repository, oid, &type,\n \t\t\t\t\t\t &size);\n+\t\tif (!contents)\n+\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n \n \t\tif (use_mailmap) {\n \t\t\tsize_t s = size;\n@@ -423,8 +429,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\tsize = cast_size_t_to_ulong(s);\n \t\t}\n \n-\t\tif (!contents)\n-\t\t\tdie(\"object %s disappeared\", oid_to_hex(oid));\n \t\tif (type != data->type)\n \t\t\tdie(\"object %s changed type!?\", oid_to_hex(oid));\n \t\tif (data->info.sizep && size != data->size && !use_mailmap)\n@@ -481,6 +485,8 @@ static void batch_object_write(const char *obj_name,\n \n \t\t\tbuf = repo_read_object_file(the_repository, &data->oid, &data->type,\n \t\t\t\t\t\t    &data->size);\n+\t\t\tif (!buf)\n+\t\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&data->oid));\n \t\t\tbuf = replace_idents_using_mailmap(buf, &s);\n \t\t\tdata->size = cast_size_t_to_ulong(s);\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex c8e33f97755..982bcfc4b1d 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,\n \n \t\t\tdata = repo_read_object_file(the_repository, &ce->oid,\n \t\t\t\t\t\t     &type, &size);\n+\t\t\tif (!data)\n+\t\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&ce->oid));\n \t\t\tinit_tree_desc(&tree, data, size);\n \n \t\t\thit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e65cae0bcf7..caf20fd5bdd 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tchar *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n \n-\t\tif (prev_buf && size)\n+\t\tif (!prev_buf)\n+\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n+\t\tif (size)\n \t\t\tstrbuf_add(&buf, prev_buf, size);\n-\t\tif (d.buf.len && prev_buf && size)\n+\t\tif (d.buf.len && size)\n \t\t\tappend_separator(&buf);\n \t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex db94581f724..d6d6fa16894 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -337,6 +337,8 @@ static char *grab_blob(struct repository *r,\n \t\tfree_filespec(df);\n \t} else {\n \t\tblob = repo_read_object_file(r, oid, &type, size);\n+\t\tif (!blob)\n+\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(oid));\n \t\tif (type != OBJ_BLOB)\n \t\t\tdie(\"object '%s' is not a blob!\", oid_to_hex(oid));\n \t}\ndiff --git a/rerere.c b/rerere.c\nindex ca7e77ba68c..13c94ded037 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,\n \t\t\tmmfile[i].ptr = repo_read_object_file(the_repository,\n \t\t\t\t\t\t\t      &ce->oid, &type,\n \t\t\t\t\t\t\t      &size);\n+\t\t\tif (!mmfile[i].ptr)\n+\t\t\t\tdie(_(\"unable to read %s\"),\n+\t\t\t\t    oid_to_hex(&ce->oid));\n \t\t\tmmfile[i].size = size;\n \t\t}\n \t}\n\nbase-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9\n-- \ngitgitgadget\n"},{"id":"487933","messageId":"CAOLa=ZQOALZRNqp7dDH0qDWoHwo6_3G8VgVuMbb3C20UdJ4C5A@mail.gmail.com","threadId":"60848","inReplyTo":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-02-05T16:10:06Z","receivedAt":"2024-02-05T16:10:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello,\n\n\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> diff --git a/bisect.c b/bisect.c\n> index f1273c787d9..f75e50c3397 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,\n>  \t\tconst char *subject_start;\n>  \t\tint subject_len;\n>\n> +\t\tif (!buf)\n> +\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&commit->object.oid));\n> +\n\nNit: We know that `repo_read_object_file()` fails on corrupt objects, so\nthis means that this is only happening when the object doesn't exist. I\nwonder if it makes more sense to replace \"unable to read %s\" which is a\nlittle ambiguous with something like \"object %q doesn't exist\".\n\nOtherwise, the patch looks good, thanks!\n"},{"id":"487975","messageId":"CAO_smVhrMn=-uF1B6+RA8A+VLCEN=o57zbQPtr8hpxRKY=qJRQ@mail.gmail.com","threadId":"60848","inReplyTo":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-06T01:13:10Z","receivedAt":"2024-02-06T01:13:27Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> There are a couple of places in Git's source code where the return value\n> is not checked. As a consequence, they are susceptible to segmentation\n> faults.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>     Always check the return value of repo_read_object_file()\n>\n>     I ran into this today, when I had tried git am -3 to import changes from\n>     a repository into a different repository that has the first repository's\n>     code vendored in. To make this work, I set\n>     GIT_ALTERNATE_OBJECT_DIRECTORIES accordingly for the git am -3 call, but\n>     forgot to set it for a subsequent git diff call, which then segfaulted.\n>\n>     There are still a couple of places left where there are checks but they\n>     look dubious to me, as they simply continue as if an empty blob had been\n>     read, for example in builtin/tag.c. However, there are checks that avoid\n>     segfaults, so I left them alone.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1650%2Fdscho%2Fsafer-object-reads-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1650/dscho/safer-object-reads-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1650\n>\n>  bisect.c           |  3 +++\n>  builtin/cat-file.c | 10 ++++++++--\n>  builtin/grep.c     |  2 ++\n>  builtin/notes.c    |  6 ++++--\n>  combine-diff.c     |  2 ++\n>  rerere.c           |  3 +++\n>  6 files changed, 22 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index e65cae0bcf7..caf20fd5bdd 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>                 struct strbuf buf = STRBUF_INIT;\n>                 char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n>\n> -               if (prev_buf && size)\n> +               if (!prev_buf)\n> +                       die(_(\"unable to read %s\"), oid_to_hex(note));\n\nThis changes the behavior of this function. Previously, it would not\nadd prev_buf output, but still succeed. This now dies.\n\n> +               if (size)\n>                         strbuf_add(&buf, prev_buf, size);\n> -               if (d.buf.len && prev_buf && size)\n> +               if (d.buf.len && size)\n>                         append_separator(&buf);\n>                 strbuf_insert(&d.buf, 0, buf.buf, buf.len);\n>\n> diff --git a/combine-diff.c b/combine-diff.c\n> index db94581f724..d6d6fa16894 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -337,6 +337,8 @@ static char *grab_blob(struct repository *r,\n>                 free_filespec(df);\n>         } else {\n>                 blob = repo_read_object_file(r, oid, &type, size);\n> +               if (!blob)\n> +                       die(_(\"unable to read %s\"), oid_to_hex(oid));\n\nTechnically this is changing the output, but I think that's good - I\nbelieve that previously the behavior was undefined, because `type`\nwouldn't be modified if the blob didn't exist, and `type` wasn't\nassigned a value earlier in the function.\n\n>                 if (type != OBJ_BLOB)\n>                         die(\"object '%s' is not a blob!\", oid_to_hex(oid));\n>         }\n>\n> base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9\n> --\n> gitgitgadget\n>\n"},{"id":"488038","messageId":"ZcHW_bc6N5umk2G4@tanuki","threadId":"60848","inReplyTo":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-06T06:51:41Z","receivedAt":"2024-02-06T06:51:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n[snip]\n> diff --git a/rerere.c b/rerere.c\n> index ca7e77ba68c..13c94ded037 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,\n>  \t\t\tmmfile[i].ptr = repo_read_object_file(the_repository,\n>  \t\t\t\t\t\t\t      &ce->oid, &type,\n>  \t\t\t\t\t\t\t      &size);\n> +\t\t\tif (!mmfile[i].ptr)\n> +\t\t\t\tdie(_(\"unable to read %s\"),\n> +\t\t\t\t    oid_to_hex(&ce->oid));\n>  \t\t\tmmfile[i].size = size;\n>  \t\t}\n>  \t}\n\nA few lines below this we check whether `mmfile[i].ptr` is `NULL` and\nreplace it with the empty string if so. So this patch here is basically\na change in behaviour where we now die instead of falling back to the\nempty value.\n\nI'm not familiar enough with the code to say whether the old behaviour\nis intended or not -- it certainly feels somewhat weird to me. But it\ndid leave me wondering and could maybe use some explanation.\n\nPatrick\n"},{"id":"488082","messageId":"xmqq8r3xign7.fsf@gitster.g","threadId":"60848","inReplyTo":"CAOLa=ZQOALZRNqp7dDH0qDWoHwo6_3G8VgVuMbb3C20UdJ4C5A@mail.gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T18:36:44Z","receivedAt":"2024-02-06T18:36:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Hello,\n>\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> diff --git a/bisect.c b/bisect.c\n>> index f1273c787d9..f75e50c3397 100644\n>> --- a/bisect.c\n>> +++ b/bisect.c\n>> @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,\n>>  \t\tconst char *subject_start;\n>>  \t\tint subject_len;\n>>\n>> +\t\tif (!buf)\n>> +\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&commit->object.oid));\n>> +\n>\n> Nit: We know that `repo_read_object_file()` fails on corrupt objects, so\n> this means that this is only happening when the object doesn't exist. I\n> wonder if it makes more sense to replace \"unable to read %s\" which is a\n> little ambiguous with something like \"object %q doesn't exist\".\n\nI am not sure if that is a good move in the longer run.  We may\n\"fix\" the called function to return NULL to allow callers to deal\nwith errors from object corruption better, at which time between\n\"doesn't exist\" and \"unable to read\", the latter becomes far closer\nto what actually happened (it is debatable if a corrupt thing really\nexists in the first place, too).\n\n> Otherwise, the patch looks good, thanks!\n\nI haven't read the remainder of the patch, but to me this hunk looks\nOK.\n\nThanks.\n"},{"id":"488084","messageId":"xmqq1q9pige1.fsf@gitster.g","threadId":"60848","inReplyTo":"ZcHW_bc6N5umk2G4@tanuki","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T18:42:14Z","receivedAt":"2024-02-06T18:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>>  \t\t\tmmfile[i].ptr = repo_read_object_file(the_repository,\n>>  \t\t\t\t\t\t\t      &ce->oid, &type,\n>>  \t\t\t\t\t\t\t      &size);\n>> +\t\t\tif (!mmfile[i].ptr)\n>> +\t\t\t\tdie(_(\"unable to read %s\"),\n>> +\t\t\t\t    oid_to_hex(&ce->oid));\n>>  \t\t\tmmfile[i].size = size;\n>>  \t\t}\n>>  \t}\n>\n> A few lines below this we check whether `mmfile[i].ptr` is `NULL` and\n> replace it with the empty string if so. So this patch here is basically\n> a change in behaviour where we now die instead of falling back to the\n> empty value.\n\nI think that one is trying to cope with cases where we genuinely do\nnot have all three variants, not \"we thought we had this variant so\nwe tried to read it into mmfile[i].{ptr,size}, but it turns out that\nthe object name we had was bad\".  So the fallback code for an entirely\ndifferent case was masking the breakage the above hunk fixes, and\nthis being \"rerere\", it is better to be cautious than sorry.\n\nThanks for reading the original code carefully.\n\n"},{"id":"488103","messageId":"xmqqplx9fdyk.fsf@gitster.g","threadId":"60848","inReplyTo":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T22:02:59Z","receivedAt":"2024-02-06T22:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/builtin/grep.c b/builtin/grep.c\n> index c8e33f97755..982bcfc4b1d 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,\n>  \n>  \t\t\tdata = repo_read_object_file(the_repository, &ce->oid,\n>  \t\t\t\t\t\t     &type, &size);\n> +\t\t\tif (!data)\n> +\t\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&ce->oid));\n>  \t\t\tinit_tree_desc(&tree, data, size);\n>  \n>  \t\t\thit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);\n\nThis caught my attention during today's integration cycle.  Checking\nnullness for data certainly is an improvement, but shouldn't we be\nchecking type as well to make sure it is a tree and not a random\ntree-ish or even blob?\n"},{"id":"488276","messageId":"5fd95ae0-cd50-ddc4-5095-ee953c2640b3@gmx.de","threadId":"60848","inReplyTo":"CAO_smVhrMn=-uF1B6+RA8A+VLCEN=o57zbQPtr8hpxRKY=qJRQ@mail.gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-09T08:06:53Z","receivedAt":"2024-02-09T08:06:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kyle,\n\nOn Mon, 5 Feb 2024, Kyle Lippincott wrote:\n\n> On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index e65cae0bcf7..caf20fd5bdd 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> >                 struct strbuf buf = STRBUF_INIT;\n> >                 char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n> >\n> > -               if (prev_buf && size)\n> > +               if (!prev_buf)\n> > +                       die(_(\"unable to read %s\"), oid_to_hex(note));\n>\n> This changes the behavior of this function. Previously, it would not\n> add prev_buf output, but still succeed. This now dies.\n\nIt does change behavior. The previous behavior looked up the note OID,\nthen tried to read it, and if it was missing just pretended that there had\nnot been a note.\n\nI'm not quite sure whether we should keep that behavior, as it is unclear\nin which scenarios it would be desirable to paper over missing objects.\n\nIn GitGitGadget, I am a heavy user of notes and it wouldn't do any good to\nhave this behavior: It would lose information.\n\nAnd even in scenarios where the `notes` ref is fetch shallowly, I would\nexpect all of the actual notes blobs to be present, and I would _want_ the\n`git note edit ...` command to error out when that blob is not found.\n\nDoes that reasoning make sense to you?\n\nCiao,\nJohannes\n"},{"id":"488278","messageId":"3f14077f-c70c-5eef-5b25-984fdf7b3b68@gmx.de","threadId":"60848","inReplyTo":"ZcHW_bc6N5umk2G4@tanuki","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-09T08:15:15Z","receivedAt":"2024-02-09T08:15:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Tue, 6 Feb 2024, Patrick Steinhardt wrote:\n\n> On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> [snip]\n> > diff --git a/rerere.c b/rerere.c\n> > index ca7e77ba68c..13c94ded037 100644\n> > --- a/rerere.c\n> > +++ b/rerere.c\n> > @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,\n> >  \t\t\tmmfile[i].ptr = repo_read_object_file(the_repository,\n> >  \t\t\t\t\t\t\t      &ce->oid, &type,\n> >  \t\t\t\t\t\t\t      &size);\n> > +\t\t\tif (!mmfile[i].ptr)\n> > +\t\t\t\tdie(_(\"unable to read %s\"),\n> > +\t\t\t\t    oid_to_hex(&ce->oid));\n> >  \t\t\tmmfile[i].size = size;\n> >  \t\t}\n> >  \t}\n>\n> A few lines below this we check whether `mmfile[i].ptr` is `NULL` and\n> replace it with the empty string if so. So this patch here is basically\n> a change in behaviour where we now die instead of falling back to the\n> empty value.\n>\n> I'm not familiar enough with the code to say whether the old behaviour\n> is intended or not -- it certainly feels somewhat weird to me. But it\n> did leave me wondering and could maybe use some explanation.\n\nHmm. That's a good point. The `mmfile[i].ptr == NULL` situation is indeed\nhandled specifically.\n\nHowever, after reading the code I come to the conclusion that the `i`\nrefers to the stage of an index entry, i.e. that loop\n(https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the\ncase where conflicts are due to deletions (where one side of the merge\ndeleted the file) or double-adds (where both sides of the merge added the\nfile, with different contents).\n\nTherefore I would suggest that ignoring missing blobs (as is the pre-patch\nbehavior) would mishandle the available data and paper over a corruption\nof the database (the blob is reachable via the Git index, but is missing).\n\nCiao,\nJohannes\n"},{"id":"488279","messageId":"ZcXft0_UbsABjlVQ@tanuki","threadId":"60848","inReplyTo":"3f14077f-c70c-5eef-5b25-984fdf7b3b68@gmx.de","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-09T08:17:59Z","receivedAt":"2024-02-09T08:18:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 09, 2024 at 09:15:15AM +0100, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Tue, 6 Feb 2024, Patrick Steinhardt wrote:\n> \n> > On Mon, Feb 05, 2024 at 02:35:53PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> > [snip]\n> > > diff --git a/rerere.c b/rerere.c\n> > > index ca7e77ba68c..13c94ded037 100644\n> > > --- a/rerere.c\n> > > +++ b/rerere.c\n> > > @@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate,\n> > >  \t\t\tmmfile[i].ptr = repo_read_object_file(the_repository,\n> > >  \t\t\t\t\t\t\t      &ce->oid, &type,\n> > >  \t\t\t\t\t\t\t      &size);\n> > > +\t\t\tif (!mmfile[i].ptr)\n> > > +\t\t\t\tdie(_(\"unable to read %s\"),\n> > > +\t\t\t\t    oid_to_hex(&ce->oid));\n> > >  \t\t\tmmfile[i].size = size;\n> > >  \t\t}\n> > >  \t}\n> >\n> > A few lines below this we check whether `mmfile[i].ptr` is `NULL` and\n> > replace it with the empty string if so. So this patch here is basically\n> > a change in behaviour where we now die instead of falling back to the\n> > empty value.\n> >\n> > I'm not familiar enough with the code to say whether the old behaviour\n> > is intended or not -- it certainly feels somewhat weird to me. But it\n> > did leave me wondering and could maybe use some explanation.\n> \n> Hmm. That's a good point. The `mmfile[i].ptr == NULL` situation is indeed\n> handled specifically.\n> \n> However, after reading the code I come to the conclusion that the `i`\n> refers to the stage of an index entry, i.e. that loop\n> (https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the\n> case where conflicts are due to deletions (where one side of the merge\n> deleted the file) or double-adds (where both sides of the merge added the\n> file, with different contents).\n> \n> Therefore I would suggest that ignoring missing blobs (as is the pre-patch\n> behavior) would mishandle the available data and paper over a corruption\n> of the database (the blob is reachable via the Git index, but is missing).\n\nYeah, that explanation sounds reasonable to me, thanks!\n\nPatrick\n"},{"id":"488300","messageId":"xmqqjzndk302.fsf@gitster.g","threadId":"60848","inReplyTo":"5fd95ae0-cd50-ddc4-5095-ee953c2640b3@gmx.de","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-09T16:37:33Z","receivedAt":"2024-02-09T16:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> It does change behavior. The previous behavior looked up the note OID,\n> then tried to read it, and if it was missing just pretended that there had\n> not been a note.\n>\n> I'm not quite sure whether we should keep that behavior, as it is unclear\n> in which scenarios it would be desirable to paper over missing objects.\n\nYeah, an object that does not have any notes attached to is a norm\nso the calling application must be prepared for it, but this\ncodepath is different.  The notes tree says it has notes, we try to\nread it and it is not there---at least we noticed an inconsistent\nnotes tree (and object store), and if we were to run \"git fsck\" at\nthat point, we would certainly complain about a missing blob object\n(can a tree object at an intermediate level be missing and would we\nnotice, by the way, I wonder).  It is only prudent to report it,\ninstead of pretending that the notes are not there.\n\nSo I think this tightening falls into the \"bugfix\" category.\n\nThanks.\n"},{"id":"488315","messageId":"CAO_smViXpcwo6_zd3iqpM175nEnH_mjca7XqJ=V4bBobY_02wQ@mail.gmail.com","threadId":"60848","inReplyTo":"5fd95ae0-cd50-ddc4-5095-ee953c2640b3@gmx.de","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-02-09T19:56:52Z","receivedAt":"2024-02-09T19:57:11Z","isPatch":true,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"On Fri, Feb 9, 2024 at 12:06 AM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Kyle,\n>\n> On Mon, 5 Feb 2024, Kyle Lippincott wrote:\n>\n> > On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> > >\n> > > diff --git a/builtin/notes.c b/builtin/notes.c\n> > > index e65cae0bcf7..caf20fd5bdd 100644\n> > > --- a/builtin/notes.c\n> > > +++ b/builtin/notes.c\n> > > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > >                 struct strbuf buf = STRBUF_INIT;\n> > >                 char *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n> > >\n> > > -               if (prev_buf && size)\n> > > +               if (!prev_buf)\n> > > +                       die(_(\"unable to read %s\"), oid_to_hex(note));\n> >\n> > This changes the behavior of this function. Previously, it would not\n> > add prev_buf output, but still succeed. This now dies.\n>\n> It does change behavior. The previous behavior looked up the note OID,\n> then tried to read it, and if it was missing just pretended that there had\n> not been a note.\n>\n> I'm not quite sure whether we should keep that behavior, as it is unclear\n> in which scenarios it would be desirable to paper over missing objects.\n>\n> In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to\n> have this behavior: It would lose information.\n>\n> And even in scenarios where the `notes` ref is fetch shallowly, I would\n> expect all of the actual notes blobs to be present, and I would _want_ the\n> `git note edit ...` command to error out when that blob is not found.\n>\n> Does that reasoning make sense to you?\n\nSounds good, thanks for talking through it with me.\n\n>\n> Ciao,\n> Johannes\n"},{"id":"488480","messageId":"805cc537-a567-e261-860c-5aba826b9e0e@gmx.de","threadId":"60848","inReplyTo":"CAOLa=ZQOALZRNqp7dDH0qDWoHwo6_3G8VgVuMbb3C20UdJ4C5A@mail.gmail.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-12T23:16:28Z","receivedAt":"2024-02-12T23:16:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Karthik,\n\nOn Mon, 5 Feb 2024, Karthik Nayak wrote:\n\n> Hello,\n>\n> \"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> > diff --git a/bisect.c b/bisect.c\n> > index f1273c787d9..f75e50c3397 100644\n> > --- a/bisect.c\n> > +++ b/bisect.c\n> > @@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,\n> >  \t\tconst char *subject_start;\n> >  \t\tint subject_len;\n> >\n> > +\t\tif (!buf)\n> > +\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&commit->object.oid));\n> > +\n>\n> Nit: We know that `repo_read_object_file()` fails on corrupt objects, so\n> this means that this is only happening when the object doesn't exist. I\n> wonder if it makes more sense to replace \"unable to read %s\" which is a\n> little ambiguous with something like \"object %q doesn't exist\".\n\nI specifically copied this error message from existing code that already\ndeals with these errors, so as not to cause unnecessary translator\nfriction.\n\nCiao,\nJohannes\n"},{"id":"488481","messageId":"fcb8240a-0b97-8554-9eee-8ff2acdb9a8f@gmx.de","threadId":"60848","inReplyTo":"xmqqplx9fdyk.fsf@gitster.g","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-12T23:19:27Z","receivedAt":"2024-02-12T23:19:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 6 Feb 2024, Junio C Hamano wrote:\n\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index c8e33f97755..982bcfc4b1d 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt,\n> >\n> >  \t\t\tdata = repo_read_object_file(the_repository, &ce->oid,\n> >  \t\t\t\t\t\t     &type, &size);\n> > +\t\t\tif (!data)\n> > +\t\t\t\tdie(_(\"unable to read tree %s\"), oid_to_hex(&ce->oid));\n> >  \t\t\tinit_tree_desc(&tree, data, size);\n> >\n> >  \t\t\thit |= grep_tree(opt, pathspec, &tree, &name, 0, 0);\n>\n> This caught my attention during today's integration cycle.  Checking\n> nullness for data certainly is an improvement, but shouldn't we be\n> checking type as well to make sure it is a tree and not a random\n> tree-ish or even blob?\n\nThat sounds right, but I am already stretching this patch beyond what I am\nfunded to work on, and therefore I need to leave it at the current state.\n\nCiao,\nJohannes\n"},{"id":"488784","messageId":"20240216064326.89551-1-tenglong.tl@alibaba-inc.com","threadId":"60848","inReplyTo":"pull.1650.git.1707143753726.gitgitgadget@gmail.com","subject":"[PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2024-02-16T06:43:26Z","receivedAt":"2024-02-16T06:43:44Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> wrote on Mon, 05 Feb 2024:\n\nHi, when I do zh_CN l10n work for 2.44, I found some check changes like:\n\n    die(_(\"unable to read tree %s\")\n\nin patchset, some old code for this like work is similar but with parentheses\nsurrounded with the OID parameter:\n\n   die(_(\"unable to read tree (%s)\")\n\nI think it's really a small nit, I don't think it's a requirement to immediately\noptimize, they're just some small printing consistency formatting issues, so make\nsome small tips here.\n\nThanks.\n"},{"id":"488893","messageId":"63f7fc07-56b3-a271-e469-e9e230c9c2ae@gmx.de","threadId":"60848","inReplyTo":"20240216064326.89551-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH] Always check the return value of `repo_read_object_file()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-18T22:36:33Z","receivedAt":"2024-02-18T22:36:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 Feb 2024, Teng Long wrote:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> wrote on Mon, 05 Feb 2024:\n>\n> Hi, when I do zh_CN l10n work for 2.44, I found some check changes like:\n>\n>     die(_(\"unable to read tree %s\")\n>\n> in patchset, some old code for this like work is similar but with parentheses\n> surrounded with the OID parameter:\n>\n>    die(_(\"unable to read tree (%s)\")\n\nFWIW I copied the error message from\nhttps://github.com/git/git/blob/v2.43.0/tree-walk.c#L103, but only now\nrealized that it is untranslated.\n\n> I think it's really a small nit, I don't think it's a requirement to immediately\n> optimize, they're just some small printing consistency formatting issues, so make\n> some small tips here.\n\nThank you for paying attention. I agree that it would be good to make\nGit's error messages consistent, even if I sadly won't be able to focus on\nthat due to changes at my dayjob.\n\nCiao,\nJohannes\n"}]}