git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC Patch] Preventing corrupt objects from entering the repository

From
MKMartin Koegler <mkoegler@auto.tuwien.ac.at>
Date
Feb 15, 2008, 07:18 UTC
Message-ID
<20080215071800.GA27772@auto.tuwien.ac.at>
In-Reply-To
<alpine.LSU.1.00.0802142345450.30505@racer.site>
On Fri, Feb 15, 2008 at 12:06:49AM +0000, Johannes Schindelin wrote:
Show 14 quoted lines
> On Thu, 14 Feb 2008, Martin Koegler wrote:
> > @@ -552,8 +553,10 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)
> >  		 */
> >  		return commit_list_insert(one, &result);
> >  
> > -	parse_commit(one);
> > -	parse_commit(two);
> > +	if (parse_commit(one))
> > +		die("invalid commit");
> > +	if (parse_commit(two))
> > +		die("invalid commit");
> 
> I'd rather have this return NULL after displaying an error.  After all, 
> merge_bases() is sort of "libified".
Would all caller of get_merge_bases treat NULL as an error?
Show 30 quoted lines
> BTW a simple "git grep parse_commit\( | grep -vwe if -e gitweb" shows 
> this:
> 
> builtin-blame.c:                        parse_commit(commit);
> builtin-checkout.c:     parse_commit(commit);
> builtin-checkout.c:                     parse_commit(old->commit);
> builtin-checkout.c:             parse_commit(new->commit);
> builtin-checkout.c:                     parse_commit(new.commit);
> builtin-describe.c:                     parse_commit(p);
> builtin-describe.c:                     parse_commit(p);
> builtin-fast-export.c:  parse_commit(commit);
> builtin-fast-export.c:          parse_commit(commit->parents->item);
> builtin-fetch-pack.c:                   parse_commit(commit);
> builtin-fetch-pack.c:                           parse_commit(commit);
> builtin-fetch-pack.c:                   parse_commit(commit);
> builtin-name-rev.c:             parse_commit(commit);
> builtin-show-branch.c:                          parse_commit(p);
> builtin-show-branch.c:          parse_commit(commit);
> commit.c:int parse_commit(struct commit *item)
> commit.c:               parse_commit(commit);
> commit.c:       parse_commit(one);
> commit.c:       parse_commit(two);
> commit.c:                       parse_commit(p);
> commit.h:int parse_commit(struct commit *item);
> contrib/gitview/gitview:                self.parse_commit(commit_lines)
> contrib/gitview/gitview:        def parse_commit(self, commit_lines):
> shallow.c:              parse_commit(commit);
> upload-pack.c:                          parse_commit((struct commit *)object);
> 
> A few of those need checking, too, I think.

Same for prepare_revision_walk. It can fail because an error in parse_XXX, but no caller checks the result.
Show 13 quoted lines
> > diff --git a/revision.c b/revision.c
> > index 6e85aaa..9f8723d 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -46,6 +46,8 @@ void add_object(struct object *obj,
> >  
> >  static void mark_blob_uninteresting(struct blob *blob)
> >  {
> > +	if (!blob)
> > +		return;
> 
> IMHO not needed [*1*].  The first user of this (static) function calls it 
> with lookup_blob(), which assumes that the blob has been read already.

What about calling handle_revision_arg with "^<tree-sha1>", where a non blob object is stored under a blob mode?

handle_revision_arg will parse only the tree and put it on the pending list. prepare_revision_walk will call mark_tree_uninteresting on it. For any object with blob mode in the tree, it calls mark_blob_uninteresting(lookup_blob(entry.sha1)); If entry.sha1 is the sha1 of an already loaded non blob object, we need this check.

Show 10 quoted lines
> [*1*] There might be a few people arguing that defensive programming is 
> never wrong.
> 
> Alas, I saw my share of defensive programming, and more often than not, 
> the real bugs were hard to find in between all those checks.  And some 
> checks were actively wrong -- again hard to see...
> 
> Remember: You can make code so simple that there are obviously no bugs. 
> And the other way is to make it so complicated that there are no obvious 
> bugs.

I would put rev_info in the second category. What can not be done with it?

Show 8 quoted lines
> > @@ -57,6 +59,8 @@ void mark_tree_uninteresting(struct tree *tree)
> >  	struct name_entry entry;
> >  	struct object *obj = &tree->object;
> >  
> > +	if (!obj)
> > +		return;
> 
> Same here.
Same argument (s/blob/tree/g).
Show 14 quoted lines
> > @@ -94,6 +98,8 @@ void mark_parents_uninteresting(struct commit *commit)
> >  
> >  	while (parents) {
> >  		struct commit *commit = parents->item;
> > +		if (!commit)
> > +			continue;
> 
> Can parents->item really be NULL?  I think not.  And indeed, a little 
> search reveals that parse_commit_buffer() does this when it constructs the 
> parents list, and encounters an invalid SHA-1:
> 
> return error("bad parents in commit %s", sha1_to_hex(item->object.sha1));
> 
> In case it is a valid SHA-1, but not a commit, it is ignored.
Its unnecessary, I missed this check.
Show 9 quoted lines
> > @@ -173,6 +179,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
> >  		struct tag *tag = (struct tag *) object;
> >  		if (revs->tag_objects && !(flags & UNINTERESTING))
> >  			add_pending_object(revs, object, tag->tag);
> > +		if (!tag->tagged)
> > +			die("bad tag");
> 
> I haven't looked yet, but I suspect that this is as impossible as the 
> invalid parent.

Eg. tag pointing to an blob, but when calling parse_tag_buffer, a non blob object is loaded under blob sha1.

But wouldn't it be simpler, if we would add
	if (!item->tagged)
		return -1;
in parse_tag_buffer?
Show 66 quoted lines
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 4179949..c25ce64 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -1943,7 +1943,8 @@ void *read_object_with_reference(const unsigned char *sha1,
> >  		}
> >  		ref_length = strlen(ref_type);
> >  
> > -		if (memcmp(buffer, ref_type, ref_length) ||
> > +		if (ref_length + 40 >= size ||
> > +		    memcmp(buffer, ref_type, ref_length) ||
> 
> Makes sense.
> 
> > @@ -494,6 +498,8 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
> >  				return error("%.*s: expected %s type, but the object dereferences to %s type",
> >  					     len, name, typename(expected_type),
> >  					     typename(o->type));
> > +			if (!o)
> > +				return -1;
> 
> You probably want to guard for ((tag *)o)->tagged == NULL; Okay, now I am 
> curious. *megoesandlooks*  Indeed, if the tag refers to an unknown type, 
> tagged = NULL.
> 
> But I think in that case, it should return an error().  And maybe only for 
> type == OBJ_TAG.
> 
> > @@ -580,6 +586,8 @@ static int handle_one_ref(const char *path,
> >  		return 0;
> >  	if (object->type == OBJ_TAG)
> >  		object = deref_tag(object, path, strlen(path));
> > +	if (!object)
> > +		return 0;
> 
> As above, it looks strange to see that object->something is checked, and 
> _then_ object.  I'd put this into curly brackets, together with the 
> deref_tag(), just to help the reader a bit, should she be as weak in mind 
> as yours truly.
> 
> > @@ -617,7 +625,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> >  		unsigned long size;
> >  
> >  		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
> > -		parse_object(commit->object.sha1);
> > +		if(!parse_object(commit->object.sha1))
> > +			continue;
> 
> Makes sense, but please add a space after the "if".
> 
> > diff --git a/tag.c b/tag.c
> > index 38bf913..990134f 100644
> > --- a/tag.c
> > +++ b/tag.c
> > @@ -9,7 +9,10 @@ const char *tag_type = "tag";
> >  struct object *deref_tag(struct object *o, const char *warn, int warnlen)
> >  {
> >  	while (o && o->type == OBJ_TAG)
> > -		o = parse_object(((struct tag *)o)->tagged->sha1);
> > +		if (((struct tag *)o)->tagged)
> > +			o = parse_object(((struct tag *)o)->tagged->sha1);
> > +		else
> > +			o = NULL;
> 
> Knowing that tagged _can_ be NULL, this makes tons of sense.  Except that 
> again, I'd call error() to tell the user, before setting o = NULL.
mfg Martin Kögler
Previous: Johannes SchindelinNext: Martin Koegler
Message 22 of 23 in “Preventing corrupt objects from entering the repository”
  1. Preventing corrupt objects from entering the repositoryMartin Koegler, Feb 10, 2008
  2. Junio C HamanoFeb 11, 2008
  3. Nicolas PitreFeb 11, 2008
  4. Martin KoeglerFeb 11, 2008
  5. Nicolas PitreFeb 11, 2008
  6. Martin KoeglerFeb 11, 2008
  7. Nicolas PitreFeb 12, 2008
  8. Martin KoeglerFeb 12, 2008
  9. Nicolas PitreFeb 12, 2008
  10. Martin KoeglerFeb 12, 2008
  11. Nicolas PitreFeb 12, 2008
  12. Shawn O. PearceFeb 13, 2008
  13. Martin KoeglerFeb 13, 2008
  14. Remove object-refs from fsckShawn O. Pearce, Feb 14, 2008
  15. Martin KoeglerFeb 14, 2008
  16. Shawn O. PearceFeb 13, 2008
  17. Martin KoeglerFeb 13, 2008
  18. Johannes SchindelinFeb 13, 2008
  19. Shawn O. PearceFeb 14, 2008
  20. Martin KoeglerFeb 14, 2008
  21. Johannes SchindelinFeb 15, 2008
  22. Martin KoeglerFeb 15, 2008
  23. Martin KoeglerFeb 12, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.