patch_parse.c: Handle CRLF in parse_header_start#5027
Conversation
|
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? |
|
I added a test which reproduces the segfault - roll back the change to Lines 931 to 940 in 5e88f13 Because the file is added (and the left side would be |
|
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. |
|
Yeah, I added the |
|
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. |
|
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)) {
```
|
|
Conflicts & style issues addressed. |
|
Thanks a lot! |
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.