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

Re: [PATCHv2 2/2] fast-import: tighten parsing of mark references

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Apr 3, 2012, 14:20 UTC
Message-ID
<20120403142001.GD15589@burratino>
In-Reply-To
<1333417910-17955-3-git-send-email-pw@padd.com>

(cc-ing Johan for noteimport code) Pete Wyckoff wrote:

>                    Fast-import does not complain when garbage
> appears after a mark reference in some cases.
Thanks for fixing it.
[...]
> +++ b/fast-import.c
[...]
Show 8 quoted lines
> @@ -2236,20 +2287,24 @@ static void file_change_m(struct branch *b)
>  
>  	if (*p == ':') {
>  		char *x;
> -		oe = find_mark(strtoumax(p + 1, &x, 10));
> +		oe = find_mark(parse_mark_ref_space(p, &x));
>  		hashcpy(sha1, oe->idx.sha1);
>  		p = x;
Simpler:
	if (*p == ':') {
		oe = find_mark(parse_mark_ref_space(p, &p));
		hashcpy(sha1, oe->idx.sha1);
	} else if ...
Show 9 quoted lines
>  	} else if (!prefixcmp(p, "inline")) {
>  		inline_data = 1;
>  		p += 6;
> +		if (*p != ' ')
> +			die("Missing space after 'inline': %s",
> +			    command_buf.buf);
>  	} else {
>  		if (get_sha1_hex(p, sha1))
>  			die("Invalid SHA1: %s", command_buf.buf);
If I write
	M 100644 inliness some/path/to/file

was my mistake actually leaving out a space after 'inline' or was it using an invalid <dataref>?

I think the latter, so I would suggest
	} else if (!prefixcmp(p, "inline ")) {
		inline_data = 1;
		p += strlen("inline");	/* advance to space */
	} else {
		if (get_sha1_hex(p, sha1))
			...
[...]
>  	}
> -	if (*p++ != ' ')
> -		die("Missing space after SHA1: %s", command_buf.buf);
> +	++p;  /* skip space */
I guess I'd suggest
	assert(*p == ' ');
	p++;

as defense against coders introducing additional cases that are not as careful.

Show 8 quoted lines
> @@ -2408,20 +2463,24 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)
>  	/* <dataref> or 'inline' */
>  	if (*p == ':') {
>  		char *x;
> -		oe = find_mark(strtoumax(p + 1, &x, 10));
> +		oe = find_mark(parse_mark_ref_space(p, &x));
>  		hashcpy(sha1, oe->idx.sha1);
>  		p = x;
Likewise (btw, why doesn't this share code with the filemodify case?):
	if (*p == ':') {
		oe = find_mark(parse_mark_with_trailing_space(p, &p));
		hashcpy(sha1, oe->idx.sha1);
	} else if ...
and so on.
[...]
Show 15 quoted lines
> @@ -2430,7 +2489,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)
>  			die("Can't add a note on empty branch.");
>  		hashcpy(commit_sha1, s->sha1);
>  	} else if (*p == ':') {
> -		uintmax_t commit_mark = strtoumax(p + 1, NULL, 10);
> +		uintmax_t commit_mark = parse_mark_ref_eol(p);
>  		struct object_entry *commit_oe = find_mark(commit_mark);
>  		if (commit_oe->type != OBJ_COMMIT)
>  			die("Mark :%" PRIuMAX " not a commit", commit_mark);
> @@ -2537,7 +2596,7 @@ static int parse_from(struct branch *b)
>  		hashcpy(b->branch_tree.versions[0].sha1, t);
>  		hashcpy(b->branch_tree.versions[1].sha1, t);
>  	} else if (*from == ':') {
> -		uintmax_t idnum = strtoumax(from + 1, NULL, 10);
> +		uintmax_t idnum = parse_mark_ref_eol(from);
The title feature.  Nice.
[...]
Show 11 quoted lines
> @@ -2945,9 +2999,7 @@ static struct object_entry *parse_treeish_dataref(const char **p)
>  
>  	if (**p == ':') {	/* <mark> */
>  		char *endptr;
> -		e = find_mark(strtoumax(*p + 1, &endptr, 10));
> -		if (endptr == *p + 1)
> -			die("Invalid mark: %s", command_buf.buf);
> +		e = find_mark(parse_mark_ref_space(*p, &endptr));
>  		if (!e)
>  			die("Unknown mark: %s", command_buf.buf);
>  		*p = endptr;
Simpler:
	if (**p == ':') {
		e = find_mark(parse_mark_...(*p, p));
		if (!e)
			die(...);
	} else {
Show 9 quoted lines
> @@ -2955,9 +3007,12 @@ static struct object_entry *parse_treeish_dataref(const char **p)
>  	} else {	/* <sha1> */
>  		if (get_sha1_hex(*p, sha1))
>  			die("Invalid SHA1: %s", command_buf.buf);
> -		e = find_object(sha1);
>  		*p += 40;
> +		if (**p != ' ')
> +			die("Missing space after SHA1: %s", command_buf.buf);
> +		e = find_object(sha1);

This seems dangerous. What if a new caller arises that wants to parse a <dataref> representing a tree-ish at the end of the line?

So I think checking the character after the tree-ish should still be the caller's responsibility.

>  	}
> +	*p += 1;  /* skip space */

If other patches in flight use the same function, they would expect *p to point to the space when parse_treeish_dataref returns. If we wanted to change that (as mentioned above I don't think we ought to) then the function's name should be changed to force such new callers not to compile.

Show 6 quoted lines
> @@ -3008,8 +3063,6 @@ static void parse_ls(struct branch *b)
>  		root = new_tree_entry();
>  		hashcpy(root->versions[1].sha1, e->idx.sha1);
>  		load_tree(root);
> -		if (*p++ != ' ')
> -			die("Missing space after tree-ish: %s", command_buf.buf);
(here's the caller).
Except where noted above, this looks good.

Thanks and hope that helps, Jonathan

Previous: Pete WyckoffNext: Pete Wyckoff
Message 14 of 22 in “fast-import: catch garbage after marks in from/merge”
  1. fast-import: catch garbage after marks in from/mergePete Wyckoff, Apr 1, 2012
  2. Jonathan NiederApr 1, 2012
  3. Pete WyckoffApr 2, 2012
  4. Dmitry IvankovApr 2, 2012
  5. Junio C HamanoApr 2, 2012
  6. Jonathan NiederApr 2, 2012
  7. Junio C HamanoApr 2, 2012
  8. 0/2 fast-import: tighten parsing of mark referencesPete Wyckoff, Apr 3, 2012
  9. 1/2 fast-import: test behavior of garbage after mark referencesPete Wyckoff, Apr 3, 2012
  10. Jonathan NiederApr 3, 2012
  11. Pete WyckoffApr 4, 2012
  12. Jonathan NiederApr 4, 2012
  13. 2/2 fast-import: tighten parsing of mark referencesPete Wyckoff, Apr 3, 2012
  14. Jonathan NiederApr 3, 2012
  15. Pete WyckoffApr 4, 2012
  16. Jonathan NiederApr 4, 2012
  17. Sverre RabbelierApr 3, 2012
  18. [PATCHv3] fast-import: tighten parsing of mark referencesPete Wyckoff, Apr 5, 2012
  19. Jonathan NiederApr 5, 2012
  20. Junio C HamanoApr 5, 2012
  21. [PATCHv4] fast-import: tighten parsing of datarefsPete Wyckoff, Apr 7, 2012
  22. Junio C HamanoApr 10, 2012

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.