Always check the return value of `repo_read_object_file()`
PATCH 1/1 Always check the return value of `repo_read_object_file()`
From: Johannes Schindelin <[email protected]>
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.
---
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(-)
bisect.c +3 -0
@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,const char *subject_start;int subject_len; +if (!buf) +die (_ ("unable to read %s" ),oid_to_hex (&commit->object.oid)); + fprintf(stderr, "%c%c%c ", (commit_flags & TREESAME) ? ' ' : 'T', (commit_flags & UNINTERESTING) ? 'U' : ' ',
builtin/cat-file.c +8 -2
@@ -221,6 +221,10 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, &type, &size);const char *target; + +if (!buffer) +die (_ ("unable to read %s" ),oid_to_hex (&oid)); +if (!skip_prefix (buffer,"object " , &target) ||get_oid_hex (target, &blob_oid))die ("%s not a valid tag" ,oid_to_hex (&oid));
@@ -416,6 +420,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d contents =repo_read_object_file (the_repository, oid, &type, &size); +if (!contents) +die ("object %s disappeared" ,oid_to_hex (oid));if (use_mailmap) {size_t s = size;
@@ -423,8 +429,6 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d size =cast_size_t_to_ulong (s); } -if (!contents) -die ("object %s disappeared" ,oid_to_hex (oid));if (type != data->type)die ("object %s changed type!?" ,oid_to_hex (oid));if (data->info.sizep && size != data->size && !use_mailmap)
@@ -481,6 +485,8 @@ static void batch_object_write(const char *obj_name, buf =repo_read_object_file (the_repository, &data->oid, &data->type, &data->size); +if (!buf) +die (_ ("unable to read %s" ),oid_to_hex (&data->oid)); buf =replace_idents_using_mailmap (buf, &s); data->size =cast_size_t_to_ulong (s);
builtin/grep.c +2 -0
@@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt, data =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!data) +die (_ ("unable to read tree %s" ),oid_to_hex (&ce->oid));init_tree_desc (&tree, data, size); hit |=grep_tree (opt, pathspec, &tree, &name,0 ,0 );
builtin/notes.c +4 -2
@@ -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)); +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);
combine-diff.c +2 -0
@@ -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));if (type !=OBJ_BLOB )die ("object '%s' is not a blob!" ,oid_to_hex (oid)); }
rerere.c +3 -0
@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate, mmfile[i].ptr =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!mmfile[i].ptr) +die (_ ("unable to read %s" ), +oid_to_hex (&ce->oid)); mmfile[i].size = size; } }
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
Signature · 1 line
gitgitgadget
Hello,
bisect.c static void show_list(const char *debug, int counted, int nr, · line 163
Quoted 11 lines · Johannes Schindelin via GitGitGadget
bisect.c +3
-0@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,const char *subject_start;int subject_len; +if (!buf) +die (_ ("unable to read %s" ),oid_to_hex (&commit->object.oid)); +
Nit: We know that repo_read_object_file() fails on corrupt objects, so this means that this is only happening when the object doesn't exist. I wonder if it makes more sense to replace "unable to read %s" which is a little ambiguous with something like "object %q doesn't exist".
Otherwise, the patch looks good, thanks!
Quoted 11 lines · Karthik Nayak
Hello,
Quoted 8 lines · Johannes Schindelin via GitGitGadget
bisect.c +0
-0@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,const char *subject_start;int subject_len;
Quoted 8 lines
bisect.c static void show_list(const char *debug, int counted, int nr, · line 163
Quoted 3 lines
bisect.c +3
-0+if (!buf) +die (_ ("unable to read %s" ),oid_to_hex (&commit->object.oid)); +Nit: We know that
repo_read_object_file()fails on corrupt objects, so this means that this is only happening when the object doesn't exist. I wonder if it makes more sense to replace "unable to read %s" which is a little ambiguous with something like "object %q doesn't exist".
I am not sure if that is a good move in the longer run. We may "fix" the called function to return NULL to allow callers to deal with errors from object corruption better, at which time between "doesn't exist" and "unable to read", the latter becomes far closer to what actually happened (it is debatable if a corrupt thing really exists in the first place, too).
Quoted 1 line
Otherwise, the patch looks good, thanks!
I haven't read the remainder of the patch, but to me this hunk looks OK.
Thanks.
Hi Karthik,
Quoted 11 lines · Karthik Nayak
Hello,
Quoted 8 lines · Johannes Schindelin via GitGitGadget
bisect.c +0
-0@@ -158,6 +158,9 @@ static void show_list(const char *debug, int counted, int nr,const char *subject_start;int subject_len;
Quoted 8 lines
bisect.c static void show_list(const char *debug, int counted, int nr, · line 163
Quoted 3 lines
bisect.c +3
-0+if (!buf) +die (_ ("unable to read %s" ),oid_to_hex (&commit->object.oid)); +Nit: We know that
repo_read_object_file()fails on corrupt objects, so this means that this is only happening when the object doesn't exist. I wonder if it makes more sense to replace "unable to read %s" which is a little ambiguous with something like "object %q doesn't exist".
I specifically copied this error message from existing code that already deals with these errors, so as not to cause unnecessary translator friction.
Ciao,
Johannes
Quoted 39 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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 <[email protected]>---
Always check the return value ofrepo_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_DIRECTORIESaccordingly 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/1650bisect.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(-)builtin/notes.c +0
-0
builtin/notes.c static int append_edit(int argc, const char **argv, const char *prefix) · line 720
Quoted 6 lines
builtin/notes.c +2
-1@@ -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.
Quoted 12 lines
builtin/notes.c +2
-1+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);combine-diff.c +0
-0
combine-diff.c static char *grab_blob(struct repository *r, · line 341
Quoted 5 lines
combine-diff.c +2
-0@@ -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.
Quoted 8 lines
combine-diff.c +0
-0if (type !=OBJ_BLOB )die ("object '%s' is not a blob!" ,oid_to_hex (oid)); }base-commit:
2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9--
gitgitgadget
Hi Kyle,
Quoted 11 lines · Kyle Lippincott
Quoted 9 lines · Johannes Schindelin via GitGitGadget
builtin/notes.c +0
-0@@ -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);
Quoted 6 lines
builtin/notes.c static int append_edit(int argc, const char **argv, const char *prefix) · line 720
Quoted 3 lines
builtin/notes.c +2
-1-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_bufoutput, but still succeed. This now dies.
It does change behavior. The previous behavior looked up the note OID, then tried to read it, and if it was missing just pretended that there had not been a note.
I'm not quite sure whether we should keep that behavior, as it is unclear in which scenarios it would be desirable to paper over missing objects.
In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to have this behavior: It would lose information.
And even in scenarios where the notes ref is fetch shallowly, I would expect all of the actual notes blobs to be present, and I would _want_ the git note edit ... command to error out when that blob is not found.
Does that reasoning make sense to you?
Ciao,
Johannes
Quoted 6 lines · Johannes Schindelin
It does change behavior. The previous behavior looked up the note OID, then tried to read it, and if it was missing just pretended that there had not been a note.
I'm not quite sure whether we should keep that behavior, as it is unclear in which scenarios it would be desirable to paper over missing objects.
Yeah, an object that does not have any notes attached to is a norm so the calling application must be prepared for it, but this codepath is different. The notes tree says it has notes, we try to read it and it is not there---at least we noticed an inconsistent notes tree (and object store), and if we were to run "git fsck" at that point, we would certainly complain about a missing blob object (can a tree object at an intermediate level be missing and would we notice, by the way, I wonder). It is only prudent to report it, instead of pretending that the notes are not there.
So I think this tightening falls into the "bugfix" category.
Thanks.
Quoted 30 lines · Johannes Schindelin
Hi Kyle,
Quoted 11 lines · Kyle Lippincott
Quoted 9 lines · Johannes Schindelin via GitGitGadget
builtin/notes.c +0
-0@@ -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);Quoted 6 lines
builtin/notes.c static int append_edit(int argc, const char **argv, const char *prefix) · line 720
Quoted 3 lines
builtin/notes.c +2
-1-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_bufoutput, but still succeed. This now dies.It does change behavior. The previous behavior looked up the note OID, then tried to read it, and if it was missing just pretended that there had not been a note.
I'm not quite sure whether we should keep that behavior, as it is unclear in which scenarios it would be desirable to paper over missing objects.
Quoted 8 lines
In GitGitGadget, I am a heavy user of notes and it wouldn't do any good to have this behavior: It would lose information.
And even in scenarios where the
notesref is fetch shallowly, I would expect all of the actual notes blobs to be present, and I would _want_ thegit note edit ...command to error out when that blob is not found.Does that reasoning make sense to you?
Sounds good, thanks for talking through it with me.
Quoted 3 lines
Ciao,
Johannes
Quoted 1 line · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
[snip]
Quoted 5 lines
rerere.c +0
-0
rerere.c static int handle_cache(struct index_state *istate, · line 981
Quoted 9 lines
rerere.c +3
-0@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate, mmfile[i].ptr =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!mmfile[i].ptr) +die (_ ("unable to read %s" ), +oid_to_hex (&ce->oid)); mmfile[i].size = size; } }
A few lines below this we check whether mmfile[i].ptr is NULL and replace it with the empty string if so. So this patch here is basically a change in behaviour where we now die instead of falling back to the empty value.
I'm not familiar enough with the code to say whether the old behaviour is intended or not -- it certainly feels somewhat weird to me. But it did leave me wondering and could maybe use some explanation.
Patrick
Quoted 6 lines · Patrick Steinhardt
Quoted 6 lines
rerere.c +3
-0mmfile[i].ptr =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!mmfile[i].ptr) +die (_ ("unable to read %s" ), +oid_to_hex (&ce->oid));
Quoted 8 lines
rerere.c static int handle_cache(struct index_state *istate, · line 981
Quoted 3 lines
rerere.c +0
-0mmfile[i].size = size; } }A few lines below this we check whether
mmfile[i].ptrisNULLand replace it with the empty string if so. So this patch here is basically a change in behaviour where we now die instead of falling back to the empty value.
I think that one is trying to cope with cases where we genuinely do not have all three variants, not "we thought we had this variant so we tried to read it into mmfile[i].{ptr,size}, but it turns out that the object name we had was bad". So the fallback code for an entirely different case was masking the breakage the above hunk fixes, and this being "rerere", it is better to be cautious than sorry.
Thanks for reading the original code carefully.
Hi Patrick,
Quoted 23 lines · Patrick Steinhardt
Quoted 1 line · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
[snip]
Quoted 5 lines
rerere.c +0
-0rerere.c static int handle_cache(struct index_state *istate, · line 981
Quoted 9 lines
rerere.c +3
-0@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate, mmfile[i].ptr =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!mmfile[i].ptr) +die (_ ("unable to read %s" ), +oid_to_hex (&ce->oid)); mmfile[i].size = size; } }A few lines below this we check whether
mmfile[i].ptrisNULLand replace it with the empty string if so. So this patch here is basically a change in behaviour where we now die instead of falling back to the empty value.
Quoted 3 lines
I'm not familiar enough with the code to say whether the old behaviour is intended or not -- it certainly feels somewhat weird to me. But it did leave me wondering and could maybe use some explanation.
Hmm. That's a good point. The mmfile[i].ptr == NULL situation is indeed handled specifically.
However, after reading the code I come to the conclusion that the i refers to the stage of an index entry, i.e. that loop
(https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the case where conflicts are due to deletions (where one side of the merge deleted the file) or double-adds (where both sides of the merge added the file, with different contents).
Therefore I would suggest that ignoring missing blobs (as is the pre-patch behavior) would mishandle the available data and paper over a corruption of the database (the blob is reachable via the Git index, but is missing).
Ciao,
Johannes
Quoted 41 lines · Johannes Schindelin
Hi Patrick,
Quoted 23 lines · Patrick Steinhardt
Quoted 1 line · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
[snip]
Quoted 5 lines
rerere.c +0
-0rerere.c static int handle_cache(struct index_state *istate, · line 981
Quoted 9 lines
rerere.c +3
-0@@ -973,6 +973,9 @@ static int handle_cache(struct index_state *istate, mmfile[i].ptr =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!mmfile[i].ptr) +die (_ ("unable to read %s" ), +oid_to_hex (&ce->oid)); mmfile[i].size = size; } }A few lines below this we check whether
mmfile[i].ptrisNULLand replace it with the empty string if so. So this patch here is basically a change in behaviour where we now die instead of falling back to the empty value.Quoted 3 lines
I'm not familiar enough with the code to say whether the old behaviour is intended or not -- it certainly feels somewhat weird to me. But it did leave me wondering and could maybe use some explanation.
Hmm. That's a good point. The
mmfile[i].ptr == NULLsituation is indeed handled specifically.However, after reading the code I come to the conclusion that the
irefers to the stage of an index entry, i.e. that loop
(https://github.com/git/git/blob/v2.43.0/rerere.c#L981-L983) handles the case where conflicts are due to deletions (where one side of the merge deleted the file) or double-adds (where both sides of the merge added the file, with different contents).
Quoted 3 lines
Therefore I would suggest that ignoring missing blobs (as is the pre-patch behavior) would mishandle the available data and paper over a corruption of the database (the blob is reachable via the Git index, but is missing).
Yeah, that explanation sounds reasonable to me, thanks!
Patrick
builtin/grep.c static int grep_cache(struct grep_opt *opt, · line 578
Quoted 13 lines
builtin/grep.c +2
-0@@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt, data =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!data) +die (_ ("unable to read tree %s" ),oid_to_hex (&ce->oid));init_tree_desc (&tree, data, size); hit |=grep_tree (opt, pathspec, &tree, &name,0 ,0 );
This caught my attention during today's integration cycle. Checking nullness for data certainly is an improvement, but shouldn't we be checking type as well to make sure it is a tree and not a random tree-ish or even blob?
Hi Junio,
Quoted 12 lines · Junio C Hamano
Quoted 12 lines
builtin/grep.c +2
-0@@ -571,6 +571,8 @@ static int grep_cache(struct grep_opt *opt, data =repo_read_object_file (the_repository, &ce->oid, &type, &size); +if (!data) +die (_ ("unable to read tree %s" ),oid_to_hex (&ce->oid));init_tree_desc (&tree, data, size);
Quoted 6 lines
builtin/grep.c static int grep_cache(struct grep_opt *opt, · line 578
Quoted 1 line
builtin/grep.c +0
-0hit |=grep_tree (opt, pathspec, &tree, &name,0 ,0 );This caught my attention during today's integration cycle. Checking nullness for data certainly is an improvement, but shouldn't we be checking type as well to make sure it is a tree and not a random tree-ish or even blob?
That sounds right, but I am already stretching this patch beyond what I am funded to work on, and therefore I need to leave it at the current state.
Ciao,
Johannes
PATCH 1/1 Always check the return value of `repo_read_object_file()`
In reply to Johannes Schindelin via GitGitGadgetJohannes Schindelin <[email protected]> wrote on Mon, 05 Feb 2024:
Hi, when I do zh_CN l10n work for 2.44, I found some check changes like:
die(_("unable to read tree %s")
in patchset, some old code for this like work is similar but with parentheses surrounded with the OID parameter:
die(_("unable to read tree (%s)")
I think it's really a small nit, I don't think it's a requirement to immediately optimize, they're just some small printing consistency formatting issues, so make some small tips here.
Thanks.
Hi,
Quoted 10 lines · Teng Long
Johannes Schindelin <[email protected]> wrote on Mon, 05 Feb 2024:
Hi, when I do
zh_CNl10n work for 2.44, I found some check changes like:die(_("unable to read tree %s")
in patchset, some old code for this like work is similar but with parentheses surrounded with the OID parameter:
die(_("unable to read tree (%s)")
FWIW I copied the error message from
https://github.com/git/git/blob/v2.43.0/tree-walk.c#L103, but only now realized that it is untranslated.
Quoted 3 lines
I think it's really a small nit, I don't think it's a requirement to immediately optimize, they're just some small printing consistency formatting issues, so make some small tips here.
Thank you for paying attention. I agree that it would be good to make Git's error messages consistent, even if I sadly won't be able to focus on that due to changes at my dayjob.
Ciao,
Johannes