Utf8JsonReader: Do not allow non-standard line endings in single-line comments by krwq · Pull Request #38036 · dotnet/corefx

krwq · 2019-05-29T23:42:23Z

Fixes: https://github.com/dotnet/corefx/issues/33321
Fixes: https://github.com/dotnet/corefx/issues/37931
(also some other fixes - see below)

Some Unicode characters (i.e. U+2028, U+2029) are considered line endings and are sometimes used in some attack vectors when coming over the wire.

This change causes that parser will throw an exception if such character is encountered in a comment.

Also fixes couple of issues w.r.t to single-line comment handling with both mult- and single segment case around EOF/final block which I haven't reported. I.e.:

end of json on \r
end of json on empty comment in the end (i.e. {}// would cause parser to throw)

Perf results are here: dotnet/performance#543

ahsonkhan

We need to throw if we see the custom separator characters in both Skip and Allow modes.

krwq · 2019-05-31T17:12:37Z

@ahsonkhan I've rewrote SkipComment logic for multi-segment almost completely and have unified Consume/Skip. Haven't did anything similar for single-segment yet.

ahsonkhan · 2019-05-31T17:45:04Z

@krwq, reviewing this would take some time and there is a bit of a churn, can this wait till next week? Do you have a sense of perf impact here?

krwq · 2019-05-31T23:27:58Z

Yes, this can wait. Since there is currently no perf tests related to comments I'll let you know when I write them 😄

krwq · 2019-06-06T22:29:16Z

Perf results are here: dotnet/performance#543
Multi-segment multiline comment specifically seems to be 2x faster for the common scenario, everything else is mostly the same. For some reason single-segment multiline comment regressed a bit (I have not changed that code path or anything related and have tried re-running tests)

ahsonkhan · 2019-06-08T04:11:38Z

Multi-segment multiline comment specifically seems to be 2x faster for the common scenario, everything else is mostly the same. For some reason single-segment multiline comment regressed a bit (I have not changed that code path or anything related and have tried re-running tests)

I ran the perf tests locally (between master and your branch) and was not able to reproduce your results:

summary:
better: 1, geomean: 1.032
worse: 9, geomean: 1.073
total diff: 10

Slower	diff/base	Base Median (ns)	Diff Median (ns)
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.15	169.56	194.15
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.14	920.21	1048.72
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.08	118.35	127.87
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.07	143.18	152.90
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.07	172.37	183.75
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.05	150.26	157.17
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.04	142.63	148.78
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.04	147.42	153.37
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.04	121.93	126.25

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing(	1.03	131.92	127.82

@adamsitnik - it looks like we are hitting a character limit on the ResultComparer output. Also, can the ResultComparer store the result to disk somewhere (by default - maybe md and csv) just like the actual perf runs (rather than us having to specify a path - maybe in one of the base/diff paths)?

adamsitnik · 2019-06-17T07:44:25Z

it looks like we are hitting a character limit on the ResultComparer output

I am not sure what would be the best way to solve it. The default output is console, which has a limited width. Most probably the easiest thing to do right now would be to shorten the ID.

System.Text.Json.Tests.Utf8JsonReaderCommentsTests.Utf8JsonReaderCommentParsing
System.Text.Json.Tests.CommentsTests.Parsing

can the ResultComparer store the result to disk somewhere

yes, you can pass the path to output csv file via --csv $filePath (full docs)

ahsonkhan · 2019-06-18T06:55:26Z

yes, you can pass the path to output csv file via --csv $filePath

Also, can the ResultComparer store the result to disk somewhere (by default - maybe md and csv) just like the actual perf runs (rather than us having to specify a path - maybe in one of the base/diff paths)?

I am aware of the --csv option. I was asking, by default, without having to pass that option in.

krwq · 2019-06-21T18:28:32Z

@ahsonkhan ping, is there anything blocking here? I'm not personally concerned about tiny (< 20%) regression in perf here considering comments are not expected to be used in high perf scenarios and there have been couple of reliability fixes here

ahsonkhan · 2019-06-22T02:26:02Z

-            return SkipMultiLineCommentMultiSegment(localBuffer.Slice(1), leftOver);
-        }
+                ReadOnlySequence<byte> commentSequence = _sequence.Slice(start, end);
+                commentSequence = commentSequence.Slice(2, commentSequence.Length - 2 - tailBytesToIgnore);


Can we try to avoid the double slicing of the ReadOnlySequence?
Maybe we can already have the start SequencePosition be off-set by 2. and change end to account for the - 2 - tailBytesToIgnore

I don't believe this is possible. SequencePosition doesn't know how to move by 2 since it may end up in different object which it doesn't know about (where ReadOnlySequence does)

ahsonkhan · 2019-06-22T02:47:03Z

is there anything blocking here?

Nope, nothing blocking.

I'm not personally concerned about tiny (< 20%) regression in perf here considering comments are not expected to be used in high perf scenarios and there have been couple of reliability fixes here

Agreed. I am still a bit concerned about the regression (especially for the "skip" mode), but we can work on perf optimizations separately.

Do you have the latest numbers based on your changes? We should double check if we have 10-20% regression on the "skip" case, because that is a bit too high.

ahsonkhan · 2019-06-25T03:23:48Z

@krwq, any updates to this?

krwq · 2019-06-25T03:40:21Z

@ahsonkhan, I have not had a chance to re-run this yet. Nothing has changes since you've run it yourself last time

ahsonkhan · 2019-06-25T03:47:21Z

I have not had a chance to re-run this yet. Nothing has changes since you've run it yourself last time

OK. Let me know when you have addressed the leftover feedback so we can merge this change in :)
Thanks.

ahsonkhan

Please resolve the merge conflict. Otherwise, LGTM.

krwq · 2019-06-26T17:07:45Z

@ahsonkhan seems the conflict is that we repeated this fix:
https://github.com/dotnet/corefx/pull/38036/files#diff-b12b49879ec5ef146f3373b69e66216bL3007

I'm building locally to double check my tests still build and run correctly after rebasing.

… comments (dotnet/corefx#38036) * Fix handling comments * Apply feedback, unify Consume and Skip for multi-segment * address some missed comments * micro changes * replace tab with spaces * apply feedback Commit migrated from dotnet/corefx@fc8bd8e

krwq requested review from WinCPP and ahsonkhan May 29, 2019 23:42

krwq changed the title ~~Utf8JsonReader: Do not allow non-standard line endings in comments~~ Utf8JsonReader: Do not allow non-standard line endings in single-line comments May 29, 2019

ahsonkhan added the area-System.Text.Json label May 29, 2019

ahsonkhan added this to the 3.0 milestone May 29, 2019

ahsonkhan reviewed May 30, 2019

View reviewed changes

ahsonkhan assigned krwq May 30, 2019

krwq force-pushed the fix-33321 branch from 3020029 to 644b771 Compare May 31, 2019 17:34

krwq commented May 31, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs

krwq force-pushed the fix-33321 branch from 0aea8e8 to 3189fd9 Compare June 6, 2019 22:23

ahsonkhan mentioned this pull request Jun 8, 2019

Modify Utf8JsonReader Comments perf test (reduce test name size and make output clearer). dotnet/performance#551

Open

ahsonkhan reviewed Jun 22, 2019

View reviewed changes

ahsonkhan approved these changes Jun 26, 2019

View reviewed changes

krwq and others added 5 commits June 26, 2019 09:57

Fix handling comments

94d18d7

Apply feedback, unify Consume and Skip for multi-segment

beae863

address some missed comments

4b4c9b2

micro changes

212e14e

replace tab with spaces

793cf1a

apply feedback

00c4c97

krwq force-pushed the fix-33321 branch from 7ef85a2 to 00c4c97 Compare June 26, 2019 17:15

krwq merged commit fc8bd8e into dotnet:master Jun 26, 2019

Conversation

krwq commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krwq commented May 31, 2019

Uh oh!

ahsonkhan commented May 31, 2019

Uh oh!

Uh oh!

krwq commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krwq commented Jun 6, 2019

Uh oh!

ahsonkhan commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamsitnik commented Jun 17, 2019

Uh oh!

ahsonkhan commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krwq commented Jun 21, 2019

Uh oh!

ahsonkhan Jun 22, 2019

Choose a reason for hiding this comment

Uh oh!

krwq Jun 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented Jun 22, 2019

Uh oh!

ahsonkhan commented Jun 25, 2019

Uh oh!

krwq commented Jun 25, 2019

Uh oh!

ahsonkhan commented Jun 25, 2019

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

krwq commented Jun 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

krwq commented May 29, 2019 •

edited

Loading

krwq commented May 31, 2019 •

edited

Loading

ahsonkhan commented Jun 8, 2019 •

edited

Loading

ahsonkhan commented Jun 18, 2019 •

edited

Loading