On Mon, Feb 5, 2024 at 6:36 AM Johannes Schindelin via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > There are a couple of places in Git's source code where the return value > is not checked. As a consequence, they are susceptible to segmentation > faults. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > --- > Always check the return value of repo_read_object_file() > > I ran into this today, when I had tried git am -3 to import changes from > a repository into a different repository that has the first repository's > code vendored in. To make this work, I set > GIT_ALTERNATE_OBJECT_DIRECTORIES accordingly for the git am -3 call, but > forgot to set it for a subsequent git diff call, which then segfaulted. > > There are still a couple of places left where there are checks but they > look dubious to me, as they simply continue as if an empty blob had been > read, for example in builtin/tag.c. However, there are checks that avoid > segfaults, so I left them alone. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1650%2Fdscho%2Fsafer-object-reads-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1650/dscho/safer-object-reads-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1650 > > bisect.c | 3 +++ > builtin/cat-file.c | 10 ++++++++-- > builtin/grep.c | 2 ++ > builtin/notes.c | 6 ++++-- > combine-diff.c | 2 ++ > rerere.c | 3 +++ > 6 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/builtin/notes.c b/builtin/notes.c > index e65cae0bcf7..caf20fd5bdd 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -716,9 +716,11 @@ static int append_edit(int argc, const char **argv, const char *prefix) > struct strbuf buf = STRBUF_INIT; > char *prev_buf = repo_read_object_file(the_repository, note, &type, &size); > > - if (prev_buf && size) > + if (!prev_buf) > + die(_("unable to read %s"), oid_to_hex(note)); This changes the behavior of this function. Previously, it would not add prev_buf output, but still succeed. This now dies. > + if (size) > strbuf_add(&buf, prev_buf, size); > - if (d.buf.len && prev_buf && size) > + if (d.buf.len && size) > append_separator(&buf); > strbuf_insert(&d.buf, 0, buf.buf, buf.len); > > diff --git a/combine-diff.c b/combine-diff.c > index db94581f724..d6d6fa16894 100644 > --- a/combine-diff.c > +++ b/combine-diff.c > @@ -337,6 +337,8 @@ static char *grab_blob(struct repository *r, > free_filespec(df); > } else { > blob = repo_read_object_file(r, oid, &type, size); > + if (!blob) > + die(_("unable to read %s"), oid_to_hex(oid)); Technically this is changing the output, but I think that's good - I believe that previously the behavior was undefined, because `type` wouldn't be modified if the blob didn't exist, and `type` wasn't assigned a value earlier in the function. > if (type != OBJ_BLOB) > die("object '%s' is not a blob!", oid_to_hex(oid)); > } > > base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9 > -- > gitgitgadget >
