Sanitize the hunk header to ensure it contains UTF-8 valid data by stanhu · Pull Request #4542 · libgit2/libgit2

stanhu · 2018-02-23T06:59:46Z

The diff driver truncates the hunk header text to 80 bytes, which
can cut off 4-byte Unicode characters and introduce garbage characters in the
diff output. This change sanitizes the hunk header before it is displayed.

This is consistent with what git does: https://github.com/git/git/blob/e3a80781f5932f5fea12a49eb06f3ade4ed8945c/diff.c#L1927-L1941

Closes libgit2/rugged#716

stanhu · 2018-02-23T08:01:50Z

Does it make sense to pull in https://github.com/stanhu/rugged-encoding-test into resources and write a test against it?

pks-t

Thanks for your PR. I've added a few remarks on your code

pks-t · 2018-02-23T09:45:03Z

 */
 extern int git__utf8_iterate(const uint8_t *str, int str_len, int32_t *dst);

+/**


As this is an internal function only, we do not want its documentation to end up public. Thus, we only "/*"-style comment headers for internal functions.

pks-t · 2018-02-23T09:46:42Z

 	return git__timer();
 }

+extern int git__utf8_valid_buf_length(const uint8_t *str, int str_len)


This function shouldn't be declared "extern" in the implementation

pks-t · 2018-02-23T09:59:28Z

+		offset += length;
+	}
+
+	return offset;


I think this function should return an ssize_t instead of an int

I actually think that this function should both take and return size_t values. At the moment, it's taking an int... which should be a small value, due to the truncation, but the caller isn't actually doing the truncation (which highlights the problem). It should ideally take a size_t which is what we use to measure the size of strings.

At that point, we should probably also return a size_t. Neither the inputs nor the outputs need to support negative values.

Right, my mistake. I was just using the prototypes from the existing UTF-8 functions.

No worries. Thanks for tackling this problem, @stanhu - I've got a busy day, but I'll try to take another pass at reviewing.

pks-t · 2018-02-23T10:07:15Z

-		info->hunk.header_len = bufs[0].size;
+		/* Sanitize the hunk header in case there is invalid Unicode */
+		buffer_len = git__utf8_valid_buf_length((const uint8_t *) bufs[0].ptr, bufs[0].size);
+		info->hunk.header_len = buffer_len;


So what you're doing here is cutting off at the point where there is the first invalid UTF8 codepoint. I think this solution here is actually not sufficient yet. In case info->hunk.header_len >= sizeof(info->hunk.header), you're actually stripping the last byte again. In case the last byte belongs to a multi-byte codepoint again, it will become invalid. So you actually have to sanitize after checking the header length.

But stepping back a bit: is it actually guaranteed that the header is in UTF8? Git is agnostic to the encoding in which files are being stored, so I could envision the diff machinery processing, say, UTF16-encoded data.

Git doesn't do well with UTF16. UTF16 files will be treated as binary (since there's a NUL) and will probably not have a header at all. I'm curious what would happen with non-UTF8 multibyte charsets, but I think it's good enough to emulate Git's behavior, which also seems to punt on those.

Yeah, I know about them being treated as binary by default. It's possible to override that via gitattributes, though. But as you say, it's probably okay to just punt on that, then. It's only non-critical metainformation, after all.

pks-t · 2018-02-23T10:09:28Z

Oh, and one more thing: I'd definitly welcome to have a test here, as you propose. Especially as the issue at hand is quite easy to reproduce

stanhu · 2018-02-24T23:25:12Z

Thanks. I addressed the review comments and added a test that passes locally in my environment, but for some reason the CI environment can't find the repository directory that I added.

UPDATE: Ok, figured it out. Missing a .gitkeep in the refs/ path.

stanhu · 2018-02-28T17:33:41Z

@pks-t Would you mind doing another round of review? Thanks!

ethomson · 2018-03-01T13:47:37Z

+
+		/* Sanitize the hunk header in case there is invalid Unicode */
+		buffer_len = git__utf8_valid_buf_length((const uint8_t *) bufs[0].ptr, bufs[0].size);
+		info->hunk.header_len = buffer_len;


I fear that this is not quite correct - previously we set info->hunk.header_len based on bufs[0].size, then truncated it if was too large to fit in info->hunk.header.

Now we're no longer doing that truncation, we're instead just setting it to the return value of git__utf8_valid_buf_length, which is looking at the valid UTF-8 character size in bufs[0].size.

The size of valid utf-8 characters in the can be larger than sizeof(info->hunk.header) (aka GIT_DIFF_HUNK_HEADER_SIZE). You can see that this change will overrun this buffer by changing GIT_DIFF_HUNK_HEADER_SIZE to a smaller value.

I thought the goal here was to cope with the truncation in the middle of a utf-8 character. In that case, should we not be giving the truncated length to the git__utf8_valid_buf_length function?

On that note, I don't understand the test then. Did you produce a file that has invalid utf-8 characters on a line, and are ensuring that they're truncated? I ask because it appears that the test is doing something, since it appears that your git__utf8_valid_buf_length function has determined that the last two bytes do not include valid bytes to make up a utf-8 character.

I expected that the test would have done the truncation at a point that produced an some bytes that were invalid UTF-8 and then truncated them. I think that test would have illustrated this problem, since there would have been no truncation at all.

But I may be misunderstanding the test...?

ethomson · 2018-03-01T16:04:32Z

+		offset += length;
+	}
+
+	return offset;


I actually think that this function should both take and return size_t values. At the moment, it's taking an int... which should be a small value, due to the truncation, but the caller isn't actually doing the truncation (which highlights the problem). It should ideally take a size_t which is what we use to measure the size of strings.

At that point, we should probably also return a size_t. Neither the inputs nor the outputs need to support negative values.

ethomson · 2018-03-01T17:12:43Z

+		"+CSSとは異なり、zIndexはすべてのオブジェクトに対してグローバルではありません。zIndexプロパティは親レイヤに対してローカルです。詳細につきましては、コンテナチュートリアルで説明しています。 [現在作業中です: Link]。\n"
+		" \n"
+		" ### スクリーン上のスプライトの順序付け方法\n"
+		" これまで学んだことを生かして、画面にスプライトを表示して、zIndexの設定をしてみましょう！\n";


Also, we've generally tried to escape these... the source character set in some VS compilers is the currently running codepage. I'm not up to speed on the ability to use utf-8 as the source character set in C proper... I actually expect that it would work but we've tried to avoid this in the past.

Ok, if you insist. I have to say 56cccc5 looks incredibly ugly. :)

pks-t

Thanks for your fixups! Here's another round of reviews

pks-t · 2018-03-02T10:35:56Z

+
+		/* Sanitize the hunk header in case there is invalid Unicode */
+		buffer_len = git__utf8_valid_buf_length((const uint8_t *) bufs[0].ptr, info->hunk.header_len);
+		info->hunk.header_len = buffer_len;


Small nit only worthwhile to fix up in case you do another round of fixes: you can avoid buffer_len by simply directly assigning to info->hunk.header_len.

pks-t · 2018-03-02T10:37:38Z

 }

+/**
+ * Iterate through an UTF-8 string and stops after finding any invalid UTF-8


Sorry, I think you misunderstood my initial comment on this documentation. It should definitely be part of the header, but instead of having a /** */ header, we want it to be a /* */ header (note the difference of one * vs two ** characters)

pks-t · 2018-03-02T10:39:25Z

+size_t git__utf8_valid_buf_length(const uint8_t *str, size_t str_len)
+{
+	size_t offset = 0;
+	int length = 0;


This initialization is unnecessary. You could also just directly declare the variable in the loop. It's only a minor nit, though, feel free to ignore this comment

pks-t · 2018-03-02T10:42:40Z

+		"index dd469c9..caf022d 100644\n"
+		"--- a/Japanese.md\n"
+		"+++ b/Japanese.md\n"
+		"@@ -24,7 +24,7 @@ Redactedでは**レイヤ**における**zIndex**属性を制御可能にする \n"


I'm a bit surprised there's a space right in front of the newline. As the test should actually show that we're correctly truncating UTF8 at the context line's cutoff point, I'd have expected the last character before the NL to be a UTF8 code point.

I am confused too. I verified that the buffer truncates the length properly; not sure where this extra space comes from yet. I'll have to dig into it later.

Ok, I figured it out. Sanitizing the buffer for valid UTF-8 characters will likely remove the newline from the hunk header, so we need to add it back if it was stripped.

stanhu · 2018-04-23T06:52:00Z

@pks-t Ok, finally found some time to figure out what was going on here and respond to your feedback. Please take a look!

pks-t

Thanks for your changes! Code looks good, and the test results look good, too. I'd love to avoid adding yet another repository for this test, though, as the diff can simply be done between two buffers instead of using two revisions. If you could change that'd be great

pks-t · 2018-04-26T11:08:45Z

 	git_buf_free(&content);
 }
+
+void test_diff_patch__can_strip_bad_utf8(void)


All that we want to do in this test is to just make sure that the diff of two files produces the correct result. So in fact we don't really need to add the complete test repository here, which would be a bit wasteful. Instead, you could just use git_diff_buffers.

I could add the two files (https://raw.githubusercontent.com/stanhu/rugged-encoding-test/3c4f7762042ad735d97d4781c4c7ee7d4486646c/Japanese.md) and (https://raw.githubusercontent.com/stanhu/rugged-encoding-test/387914a182ae0868364ec09fadb1beb47690fd4a/Japanese.md) in as character buffers, but wouldn't this pollute the test even more since we have to escape UTF-8 characters?

Ideally I'd be able to simplify the test case so that it boils down to a few lines, but when I tried earlier it was challenging to reproduce. I can try again.

@pks-t Ok, I think I figured out a better test case. I just mirrored this one: https://github.com/git/git/blob/master/t/t4025-hunk-header.sh

Oh, these tests are great! Thanks a lot!

The diff driver truncates the hunk header text to 80 bytes, which can truncate 4-byte Unicode characters and introduce garbage characters in the diff output. This change sanitizes the hunk header before it is displayed. This mirrors the test in git: https://github.com/git/git/blob/master/t/t4025-hunk-header.sh Closes libgit2/rugged#716

pks-t · 2018-05-07T11:44:24Z

Thanks a lot for this PR!

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch from f67e79c to 5da325a Compare February 23, 2018 08:02

pks-t requested changes Feb 23, 2018

View reviewed changes

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch from b15e3a6 to c17e760 Compare February 24, 2018 18:37

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch from 8535013 to a2dff52 Compare February 25, 2018 06:06

ethomson reviewed Mar 1, 2018

View reviewed changes

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch 2 times, most recently from 815be7c to b0e39cb Compare March 2, 2018 04:48

pks-t requested changes Mar 2, 2018

View reviewed changes

ethomson added the next release label Mar 10, 2018

ethomson mentioned this pull request Mar 10, 2018

Release v0.27.0 & v0.26.1 #4465

Closed

13 tasks

ethomson removed the next release label Mar 16, 2018

pks-t added the feedback provided label Mar 26, 2018

pks-t requested changes Apr 26, 2018

View reviewed changes

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch from f5774ed to 26b37e5 Compare May 5, 2018 21:52

stanhu force-pushed the sh-sanitize-utf8-hunk-header branch from 26b37e5 to 9d83a2b Compare May 5, 2018 21:54

pks-t approved these changes May 7, 2018

View reviewed changes

pks-t merged commit bb468ad into libgit2:master May 7, 2018

stanhu mentioned this pull request Jan 4, 2019

Expected date for next release? #4923

Closed

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Uh oh!

Conversation

stanhu commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stanhu commented Feb 23, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Feb 23, 2018

Uh oh!

stanhu commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stanhu commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stanhu commented Apr 23, 2018

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stanhu Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented May 7, 2018

Uh oh!

Reviewers

stanhu commented Feb 23, 2018 •

edited

Loading

stanhu commented Feb 24, 2018 •

edited

Loading

ethomson Mar 1, 2018 •

edited

Loading

stanhu Apr 26, 2018 •

edited

Loading