Re: [PATCH] range-diff: support reading mbox files

> On 15/11/2022 18:20, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Internally, the `git range-diff` command spawns a `git log` process and
> > parses its output for the given commit ranges.
> >
> > This works well when the patches that need to be compared are present in
> > the local repository in the form of commits.
> >
> > In scenarios where that is not the case, the `range-diff` command is
> > currently less helpful.
> >
> > The Git mailing list is such a scenario: Instead of using Git to
> > exchange commits, the patches are sent there as plain-text and no commit
> > range can be specified to let `range-diff` consume those patches.
> >
> > Instead, the expectation is to download the mails, apply them locally
> > and then use `range-diff`. This can be quite cumbersome e.g. when a
> > suitable base revision has to be found first where the patch applies
> > cleanly.
>
> That's a good motivation for this change.

> > +static inline int strtost(char const *s, size_t *result, const char **end)
> > +{
> > +	unsigned long u;
> > +	char *p;
> > +
> > +	errno = 0;
> > +	/* negative values would be accepted by strtoul */
> > +	if (*s == '-')
> > +		return -1;
>
> I think it is right to treat the input as untrusted and so look for malformed
> hunk headers. However This test is not sufficient for that, we expect a digit
> so I think
>
> 	if (!isdigit(*s))
> 		return -1;
>
> would be safer. The use of strtoul() looks good as we set errno to zero before
> the call and check both errno and endp afterwards.

>
> > +	u = strtoul(s, &p, 10);
> > +	if (errno || p == s)
> > +		return -1;
> > +	if (result)
> > +		*result = u;
> > +	*end = p;
> > +
> > +	return 0;
> > +}
> > +
> > +static int parse_hunk_header(const char *p,
> > +			     size_t *old_count, size_t *new_count,
> > +			     const char **end)
> > +{
> > +	size_t o = 1, n = 1;
> > +
> > +	if (!skip_prefix(p, "@@ -", &p) ||
> > +	    strtost(p, NULL, &p) ||
> > +	    (*p != ' ' && (*p != ',' || strtost(p + 1, &o, &p))) ||
>
> It took me a minute to understand the double negatives but it is correctly
> checking if we have ' ' or ',<digits>'

>
> > +	    !skip_prefix(p, " +", &p) ||
> > +	    strtost(p, NULL, &p) ||
> > +	    (*p != ' ' && (*p != ',' || strtost(p + 1, &n, &p))) ||
> > +	    !skip_prefix(p, " @@", &p))
> > +		return -1;
> > +
> > +	*old_count = o;
> > +	*new_count = n;
> > +	*end = p;
> > +
> > +	return 0;
> > +}
> > +
> > +static inline int find_eol(const char *line, size_t size)
> > +{
> > +	char *eol;
> > +
> > +	eol = memchr(line, '\n', size);
> > +	if (!eol)
> > +		return size;
> > +
> > +	if (eol != line && eol[-1] == '\r')
> > +		eol[-1] = '\0';
> > +	else
> > +		*eol = '\0';
> > +
> > +	return eol + 1 - line;
>
> We return the offset to the start of the next line, not the length of the
> line. This will be important later.

>
> > +}
> > +
> > +static int read_mbox(const char *path, struct string_list *list)
> > +{
> > +	struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
> > +	struct strbuf long_subject = STRBUF_INIT;
> > +	struct patch_util *util = NULL;
> > +	enum {
> > +		MBOX_BEFORE_HEADER,
> > +		MBOX_IN_HEADER,
> > +		MBOX_IN_COMMIT_MESSAGE,
> > +		MBOX_AFTER_TRIPLE_DASH,
> > +		MBOX_IN_DIFF
> > +	} state = MBOX_BEFORE_HEADER;
> > +	char *line, *current_filename = NULL;
> > +	int len;
> > +	size_t size, old_count = 0, new_count = 0;
> > +	const char *author = NULL, *subject = NULL;
> > +
> > +	if (!strcmp(path, "-")) {
> > +		if (strbuf_read(&contents, STDIN_FILENO, 0) < 0)
> > +			return error_errno(_("could not read stdin"));
> > +	} else if (strbuf_read_file(&contents, path, 0) < 0)
> > +		return error_errno(_("could not read '%s'"), path);
> > +
> > +	line = contents.buf;
> > +	size = contents.len;
> > +	for (; size > 0; size -= len, line += len) {
>
> size is unsigned so we're effectively testing 'size != 0' which means if we're
> off by one somewhere we'll have an undetected buffer overflow. Using a signed
> type wouldn't prevent the buffer overflow but it would limit its extent.

>
> > +		const char *p;
> > +
> > +		len = find_eol(line, size);
>
> Here len is not the length of line if it originally ended "\r\n".
>
> > +		if (state == MBOX_BEFORE_HEADER) {
> > +			if (!skip_prefix(line, "From ", &p))
> > +				continue;
> > +
> > +			util = xcalloc(1, sizeof(*util));
> > +			if (get_oid_hex(p, &util->oid) < 0)
> > +				oidcpy(&util->oid, null_oid());
> > +			util->matching = -1;
> > +			author = subject = NULL;
> > +
> > +			state = MBOX_IN_HEADER;
>
> I wondered if there should there be a `continue;` here but I think it probably
> needs to "fall-through" to the MBOX_IN_HEADER handling below. A comment to
> clarify that would be helpful.

>
> > +		}
> > +
> > +		if (starts_with(line, "diff --git ")) {
> > +			struct patch patch = { 0 };
> > +			struct strbuf root = STRBUF_INIT;
> > +			int linenr = 0;
> > +			int orig_len;
> > +
> > +			state = MBOX_IN_DIFF;
> > +			old_count = new_count = 0;
> > +			strbuf_addch(&buf, '\n');
> > +			if (!util->diff_offset)
> > +				util->diff_offset = buf.len;
> > +			line[len - 1] = '\n';
>
> Here the line will still be NUL terminated if it originally ended "\r\n" which
> presumably messes up the call to parse_git_diff_header() below. I have not
> checked if parse_git_diff_header() can handle "\r\n" when it is parsing the
> rest of the diff header.

>
> > +			orig_len = len;
> > +			len = parse_git_diff_header(&root, &linenr, 1, line,
> > +						    len, size, &patch);
> > +			if (len < 0) {
> > +				error(_("could not parse git header '%.*s'"),
> > +				      orig_len, line);
> > +				free(util);
> > +				free(current_filename);
> > +				string_list_clear(list, 1);
> > +				strbuf_release(&buf);
> > +				strbuf_release(&contents);
> > +				strbuf_release(&long_subject);
> > +				return -1;
> > +			}
> > +
> > +			if (patch.old_name)
> > +				skip_prefix(patch.old_name, "a/",
> > +					    (const char **)&patch.old_name);
> > +			if (patch.new_name)
> > +				skip_prefix(patch.new_name, "b/",
> > +					    (const char **)&patch.new_name);
>
> I think this is fine for now but we might want to support other prefixes in
> the future. If it is not a copy or rename then the filename can be deduced by
> finding the common tail of patch.old_name and patch.new_name and stripping
> anything before the first '/'. If it is a copy or rename then I suspect there
> is no prefix (though I've not checked)

>
> > +			strbuf_addstr(&buf, " ## ");
> > +			if (patch.is_new > 0)
>
> `patch.is_now` and `patch.is_delete` are booleans like `patch.is_rename` so we
> don't need the '> 0'

>
> > +				strbuf_addf(&buf, "%s (new)", patch.new_name);
> > +			else if (patch.is_delete > 0)
> > +				strbuf_addf(&buf, "%s (deleted)",
> > patch.old_name);
> > +			else if (patch.is_rename)
> > +				strbuf_addf(&buf, "%s => %s", patch.old_name,
> > patch.new_name);
> > +			else
> > +				strbuf_addstr(&buf, patch.new_name);
> > +
> > +			free(current_filename);
> > +			if (patch.is_delete > 0)
> > +				current_filename = xstrdup(patch.old_name);
> > +			else
> > +				current_filename = xstrdup(patch.new_name);
> > +
> > +			if (patch.new_mode && patch.old_mode &&
> > +			    patch.old_mode != patch.new_mode)
> > +				strbuf_addf(&buf, " (mode change %06o =>
> > %06o)",
> > +					    patch.old_mode, patch.new_mode);
> > +
> > +			strbuf_addstr(&buf, " ##\n");
> > +			util->diffsize++;
> > +		} else if (state == MBOX_IN_HEADER) {
> > +			if (!line[0]) {
> > +				state = MBOX_IN_COMMIT_MESSAGE;
> > +				/* Look for an in-body From: */
> > +				if (size > 5 && skip_prefix(line + 1, "From:
> > ", &p)) {
>
> The "size > 5" seems a bit unnecessary as we're using skip_prefix()

>
> > +					size -= p - line;
> > +					line += p - line;
>
> This is good, we're accounting for reading the next line.
>
> > +					len = find_eol(line, size);
> > +
> > +					while (isspace(*p))
> > +						p++;
> > +					author = p;
> > +				}
> > +				strbuf_addstr(&buf, " ## Metadata ##\n");
> > +				if (author)
> > +					strbuf_addf(&buf, "Author: %s\n",
> > author);
> > +				strbuf_addstr(&buf, "\n ## Commit message
> > ##\n");
> > +				if (subject)
> > +					strbuf_addf(&buf, "    %s\n\n",
> > subject);
> > +			} else if (skip_prefix(line, "From: ", &p)) {
> > +				while (isspace(*p))
> > +					p++;
> > +				author = p;
> > +			} else if (skip_prefix(line, "Subject: ", &p)) {
> > +				const char *q;
> > +
> > +				while (isspace(*p))
> > +					p++;
> > +				subject = p;
> > +
> > +				if (starts_with(p, "[PATCH") &&
> > +				    (q = strchr(p, ']'))) {
> > +					q++;
> > +					while (isspace(*q))
> > +						q++;
> > +					subject = q;
> > +				}
> > +
> > +				if (len < size && line[len] == ' ') {
> > +					/* handle long subject */
> > +					strbuf_reset(&long_subject);
> > +					strbuf_addstr(&long_subject, subject);
> > +					while (len < size && line[len] == ' ')
> > {
> > +						line += len;
> > +						size -= len;
> > +						len = find_eol(line, size);
> > +						strbuf_addstr(&long_subject,
> > line);
>
> Looks good
>
> > +					}
> > +					subject = long_subject.buf;
> > +				}
> > +			}
> > +		} else if (state == MBOX_IN_COMMIT_MESSAGE) {
> > +			if (!*line)
>
> Not a big issue elsewhere you've used "!line[0]"
> Style: there should be braces on this branch.

> [... addressed in follow-up mail...]
>
> I think this is a useful addition, it could perhaps benefit from more tests
> though. Having tests for bad input, "\r\n" line endings and getting the author
> from a From: header as well as an in-body From: line would give a bit more
> reassurance about how robust the parser is.