patch_parse.c: Handle CRLF in parse_header_start

[ddevault]

This becomes a segfault later (in check_prefix) when parsing patches where a file was added. CRLF seems to be handled fine otherwise. Added test which demonstrates the segfault.

Supporting CRLF is important because patches are often emailed - and emails use CRLF.

[ethomson]

Why does this segfault later? It's not obvious to me how this would prevent that, so I'm concerned that there's two issues here (the CRLF issue something causing else causing a segfault). Can you provide reproduction steps?

[ddevault]

I added a test which reproduces the segfault - roll back the change to patch_parse.c and run the test suite to reproduce. The issue is that if this branch is taken, it sets the header_old_path to NULL, assuming that it just parsed some bunk data and hoping the --- line will clear things up (per the comments nearby) - in this case, it doesn't, because the --- is /dev/null - no prefix. The resulting segfault happens here:

libgit2/src/patch_parse.c

Lines 931 to 940 in 5e88f13

	prefixed_old = (!added && patch->old_path) ? patch->old_path :
	patch->header_old_path;
	prefixed_new = (!deleted && patch->new_path) ? patch->new_path :
	patch->header_new_path;
	if (check_prefix(
	&patch->old_prefix, &old_prefixlen, patch, prefixed_old) < 0 ||
	check_prefix(
	&patch->new_prefix, &new_prefixlen, patch, prefixed_new) < 0)
	return -1;

Because the file is added (and the left side would be /dev/null and not have a prefix), this chooses to use header_old_path - which was erroneously set to NULL earlier.

[eaigner]

@ethomson @ddevault it seems the underlying issue is something else.

git__isspace should consume any of SPACE, \t, \n, \f, \r or \v which makes me skeptical that the issue is related to \r at all.

I fixed a SPACE issue here #5030

[ddevault]

Hm, that doesn't seem related. There are no spaces in the paths in the test case I added, and when I add my test case to your patch it still segfaults.

[eaigner]

Yeah, I added the \r at the end to test and it segfaults

[pks-t]

Yeah, I added the \r at the end to test and it segfaults

You mean when testing against your #5030? For what it's worth, it does work just fine with #5035 (at least there's no segfault there, the test still fails due to mis-parsing).

[eaigner]

It would segfault before my patch. Works with my patch. But I'm fine with either solution. I don't care about it being properly parsed, just that it doesn't crash my app.

[pks-t]

@ddevault, could you please rebase the PR to fix the conflict? There's two minor stylistic issues -- I don't care too much about the stylistic issue in the tests, but the other one should probably be fixed before merging this.

[pks-t]

On Fri, Apr 05, 2019 at 03:18:47PM +0000, Drew DeVault wrote: ddevault commented on this pull request. > @@ -328,7 +328,8 @@ static int parse_header_start(git_patch_parsed *patch, git_patch_parse_ctx *ctx) * proceeed here. We then hope for the "---" and "+++" lines to fix that * for us. */ - if (!git_parse_ctx_contains(&ctx->parse_ctx, "\n", 1)) { + if (!git_parse_ctx_contains(&ctx->parse_ctx, "\n", 1) + && !git_parse_ctx_contains(&ctx->parse_ctx, "\r\n", 2)) { Should I align with a mix of tabs and spaces?

Yes, please. We basically use git.git's style, which is in turn built on linux.git's style. Tabs are expected to be 8 spaces, so the above would be indented by 1 tab and 4 spaces: ``` if (!git_parse_ctx_contains(&ctx->parse_ctx, "\n", 1) && !git_parse_ctx_contains(&ctx->parse_ctx, "\r\n", 2)) { ```

[ddevault]

Conflicts & style issues addressed.

[pks-t]

Thanks a lot!