[PATCH v2] Make mark parsing much more restrictive

["Jörg Sommer" <joerg@alea.gnuu.de>]

The current implementation of mark parsing doesn't care for trailing
garbage like in :12a and doesn't check for unsigned numbers, i.e. it
accepts :-12 as a valid mark.

This patch enforces a number follows the colon and there comes nothing
after the bignum.

Signed-off-by: Jörg Sommer <joerg@alea.gnuu.de>
---
 fast-import.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 38 insertions(+), 11 deletions(-)

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Jrg Sommer <joerg@alea.gnuu.de> wrote:
> >
> > Another question: Is :0 a valid mark? In import_marks() is a check for
> > !mark, but I haven't seen it anywhere else.
> 
> No, in fast-import ":0" is _not_ a valid mark.

Then I propose the following patch.

diff --git a/fast-import.c b/fast-import.c
index 73e5439..0c71da8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1690,12 +1690,31 @@ static void skip_optional_lf(void)
 		ungetc(term_char, stdin);
 }
 
+static inline uintmax_t parse_mark(const const char *str, char **after_mark)
+{
+	char *am;
+	uintmax_t m;
+
+	if (!str || str[0] != ':' || !isdigit(str[1]))
+		return 0;
+
+	m = strtoumax(&str[1], &am, 10);
+	if (m != UINTMAX_MAX || errno == 0) {
+		*after_mark = am;
+		return m;
+	}
+	return 0;
+}
+
 static void cmd_mark(void)
 {
-	if (!prefixcmp(command_buf.buf, "mark :")) {
-		next_mark = strtoumax(command_buf.buf + 6, NULL, 10);
+	uintmax_t mark;
+	char *after_mark = NULL;
+
+	if (!prefixcmp(command_buf.buf, "mark ") &&
+		(next_mark = parse_mark(&command_buf.buf[5], &after_mark)) &&
+		*after_mark == '\0')
 		read_next_command();
-	}
 	else
 		next_mark = 0;
 }
@@ -1877,8 +1896,8 @@ static void file_change_m(struct branch *b)
 	}
 
 	if (*p == ':') {
-		char *x;
-		oe = find_mark(strtoumax(p + 1, &x, 10));
+		char *x = NULL;
+		oe = find_mark(parse_mark(p, &x));
 		hashcpy(sha1, oe->sha1);
 		p = x;
 	} else if (!prefixcmp(p, "inline")) {
@@ -2045,7 +2064,10 @@ static int cmd_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);
+		char *after_mark;
+		uintmax_t idnum = parse_mark(from, &after_mark);
+		if (!idnum || *after_mark != '\0')
+			die("Not a valid mark: %s", from);
 		struct object_entry *oe = find_mark(idnum);
 		if (oe->type != OBJ_COMMIT)
 			die("Mark :%" PRIuMAX " not a commit", idnum);
@@ -2080,7 +2102,10 @@ static struct hash_list *cmd_merge(unsigned int *count)
 		if (s)
 			hashcpy(n->sha1, s->sha1);
 		else if (*from == ':') {
-			uintmax_t idnum = strtoumax(from + 1, NULL, 10);
+			char *after_mark;
+			uintmax_t idnum = parse_mark(from, &after_mark);
+			if (!idnum || *after_mark != '\0')
+				die("Not a valid mark: %s", from);
 			struct object_entry *oe = find_mark(idnum);
 			if (oe->type != OBJ_COMMIT)
 				die("Mark :%" PRIuMAX " not a commit", idnum);
@@ -2228,7 +2253,10 @@ static void cmd_new_tag(void)
 		hashcpy(sha1, s->sha1);
 	} else if (*from == ':') {
 		struct object_entry *oe;
-		from_mark = strtoumax(from + 1, NULL, 10);
+		char *after_mark;
+		from_mark = parse_mark(from, &after_mark);
+		if (!from_mark || *after_mark != '\0')
+			die("Not a valid mark: %s", from);
 		oe = find_mark(from_mark);
 		if (oe->type != OBJ_COMMIT)
 			die("Mark :%" PRIuMAX " not a commit", from_mark);
@@ -2333,9 +2361,8 @@ static void import_marks(const char *input_file)
 		if (line[0] != ':' || !end)
 			die("corrupt mark line: %s", line);
 		*end = 0;
-		mark = strtoumax(line + 1, &end, 10);
-		if (!mark || end == line + 1
-			|| *end != ' ' || get_sha1(end + 1, sha1))
+		mark = parse_mark(line, &end);
+		if (!mark || *end != ' ' || get_sha1(end + 1, sha1))
 			die("corrupt mark line: %s", line);
 		e = find_object(sha1);
 		if (!e) {
-- 
1.5.5.1