Scanner: Generate error on inbalanced RLO/LRO/PDF override markers. #10326

christianparpart · 2020-11-18T13:39:05Z

Implements nested counting on RLO/LRO/PDF

Refs #10254

TODO:

changelog entry
tests for single line comments, multiline comments, too many pushes, and too many pops.
validate RLO/RLO/PDO (including RLE/LRE)
validate for RLI/LRI/PDI
use validators in string literals and have tests for them.

liblangutil/Scanner.cpp

cameel

I'm not finished yet but the call is coming up so here's the first batch.

Also, some general remarks:

Isn't this a breaking change?
What about PDI and LRI/RLI? Do we want to handle these too?

Changelog.md

liblangutil/Scanner.cpp

liblangutil/Scanner.h

liblangutil/Scanner.cpp

cameel

The rest of my review.

liblangutil/Scanner.cpp

scripts/test_antlr_grammar.sh

test/libsolidity/syntaxTests/comments/multiline_unicode_direction_override_1.sol

test/libsolidity/syntaxTests/comments/singleline_unicode_direction_override_6.sol

liblangutil/Scanner.cpp

stackenbotten · 2020-11-25T13:14:32Z

There was an error when running chk_coding_style for commit 8f428cf37843addce4ae668071c411d30b832e4f:

Coding style error:
 liblangutil/Scanner.cpp:306: for (auto const& seq : boost::make_iterator_range(m_pushSequenceBegin, m_pushSequencesEnd))
 liblangutil/Scanner.cpp:292: UnicodeDirectionProcessor(Scanner& _scanner, string_view _pop, string_view const* _pushBegin, string_view const*_pushEnd):

Please check that your changes are working as intended.

stackenbotten · 2020-12-15T14:20:43Z

There was an error when running chk_coding_style for commit 54e215f4b95c026262fa45bd0a77c30d60b5bb5f:

Error: Trailing whitespace found:
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:1:contract TimelockUpgrade {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:2: uint256 public upgradeM;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:3: uint256 public upgradeD;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:4: address public masterCopy;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:5: address public proposedMaster;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:6: event UpgradeProposed(address proposed, uint256 month, uint256 day);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:7: event UpgradeConfirmed(address newMaster);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:8:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:9: function proposeUpgrade(address _upgradeAddress) external onlyOwner^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:10: {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:11: uint256 m; ^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:12: uint256 d;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:13: (/*year*/, /*month*/ m, /*day*/ d, /*hour*/, /*minute*/, /*second*/) = BokkyDateTime.timestampToDateTime(block.timestamp);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:14:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:15: upgradeM = (m % 12) + 1; // If it's Dec, advance to Jan.^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:16: upgradeD = d > 28 ? 28 : d; // Protect for February.^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:17:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:18: proposedMaster = _upgradeAddress;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:19: }^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:20:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:21: function confirmUpgrade() external^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:22: {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:23: uint256 m;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:24: uint256 d;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:25: (/*year*/, /*monthM-bM-^@M-.*/ ,d /*yad*/ ,m /*M-bM-^@M-,M-bM-^@M-,hour*/, /*minute*/, /*second*/) = BokkyDateTime.timestampToDateTime(block.timestamp);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:26:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:27: if (m == upgradeM && d == upgradeD) {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:28: masterCopy = proposedMaster;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:29: emit UpgradeConfirmed(masterCopy);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:30: }^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:31: }^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:32:}^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:33:^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:34:// ----^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:35:// ParserError 8936: (1253-1264): Mismatching directional override markers in comment or string literal.^M

Please check that your changes are working as intended.

stackenbotten · 2020-12-15T14:45:19Z

There was an error when running chk_coding_style for commit cfee4a3bedc5644a50e27fff09874c6233b32695:

Error: Trailing whitespace found:
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:1:contract TimelockUpgrade {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:2: function confirmUpgrade() external {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:3: uint256 m;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:4: uint256 d;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:5: (/*year*/,/*monthM-bM-^@M-.*/,d/*yad*/,m/*M-bM-^@M-,M-bM-^@M-,hour*/,/*minute*/,/*second*/) = BokkyDateTime.timestampToDateTime(block.timestamp);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:6: }^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:7:}^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:8:^M

Please check that your changes are working as intended.

stackenbotten · 2020-12-15T14:48:24Z

There was an error when running chk_coding_style for commit 89d09cd2a563466495dc0cdd668b5181530054e5:

Error: Trailing whitespace found:
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:1:contract TimelockUpgrade {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:2: function confirmUpgrade() external {^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:3: uint256 m;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:4: uint256 d;^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:5: (/*year*/,/*monthM-bM-^@M-.*/,d/*yad*/,m/*M-bM-^@M-,M-bM-^@M-,hour*/,/*minute*/,/*second*/) = BokkyDateTime.timestampToDateTime(block.timestamp);^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:6: }^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:7:}^M
 test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol:8:^M

Please check that your changes are working as intended.

liblangutil/Scanner.cpp

Changelog.md

chriseth · 2020-12-15T15:45:52Z

test/libyul/yulSyntaxTests/invalid/unicode_string_direction_override.sol

+    let s := unicode"underflow ‬";
+}
+// ----
+// ParserError 1856: (35-47): Literal or identifier expected.


Is this the right error?

Looks like AsmParser doesn't accept unicode directional sequences.
It fails as currentToken() returns 'Token::Illegal'. So, I would say it is fine for now, so it is not even possible to get this problem in yul, but we would need to decide how to proceed on this in yul...

I created issue so we can discuss this topic #10622

axic · 2020-12-15T15:47:58Z

From #10326 (comment)

Legitimacy is an arguable point, but real Arabic/Hebrew/etc keyboards can't type BiDi control characters, so you are not likely to come across them in non-malicious user input.

If this is true, why do we bother? Why don't we reject them for 0.8.0 and consult more with relevant people? (could have asked about language use in comments/literals on the user survey -- cc @franzihei)

mijovic · 2020-12-15T16:06:48Z

I added few more tests, hope it is good coverage now

cameel · 2020-12-15T16:21:47Z

The biggest problem with this PR is basically that it's hard to know for sure so we try to be careful not to accidentally forbid something that turns out to be common. Asking people is definitely a good idea but it will not be the ultimate answer. Even if typical keyboards do not have this, some IMEs could. And AFAIK these features in Unicode are (relatively) new, especially LRI/RLI/PDI (that we don't even support here) so they could just not be in use yet. At this point I'm more or less convinced that it's rarely used in practice but this is still a conclusion supported mostly by lack of information :)

Also, to be honest, just attempting to push this through forced us (at least me) to explore the topic and form an opinion. Now I think that @ekpyron's proposal from #10607 in some form is the way to go but it's a bit too late to switch to that and still get it included in 0.8.0. I think that this PR is good enough if we want to have at least something to protect against the trick from the underhanded contest. We can switch to something better thought out in 0.9.0.

axic · 2020-12-15T16:31:41Z

I was always on the opinion to just outright reject most of this complexity: #10254 (comment)

But as discussed in #10607 unicode might be introducing new feature we can not prepare for, so there is always a risk.

cameel

Seems mostly fine to me. The only remarks I have are minor text/naming tweaks (sorry for the one the changelog; seems like it's the tenth time or so you're tweaking it :P).

I wonder if a test for using that in imported file name or multisource-test file name would be useful. I guess it's still just a string literal so probably not...

cameel · 2020-12-15T16:24:00Z

liblangutil/Scanner.cpp

+			pair<string_view, int>{"\xE2\x80\xAE", 1}, // U+202E (RLO - Right-to-Left Override)
+			pair<string_view, int>{"\xE2\x80\xAA", 1}, // U+202A (LRE - Left-to-Right Embedding)
+			pair<string_view, int>{"\xE2\x80\xAB", 1}, // U+202B (RLE - Right-to-Left Embedding)
+			pair<string_view, int>{"\xE2\x80\xAC", -1} // PDF - Pop Directional Formatting


Suggested change

pair<string_view, int>{"\xE2\x80\xAC", -1} // PDF - Pop Directional Formatting

pair<string_view, int>{"\xE2\x80\xAC", -1} // U+202C (PDF - Pop Directional Formatting)

cameel · 2020-12-15T16:27:30Z

scripts/check_style.sh

-WHITESPACE=$(git grep -n -I -E "^.*[[:space:]]+$" | grep -v "test/libsolidity/ASTJSON\|test/libsolidity/ASTRecoveryTests\|test/compilationTests/zeppelin/LICENSE")
+WHITESPACE=$(git grep -n -I -E "^.*[[:space:]]+$" |
+  grep -v "test/libsolidity/ASTJSON\|test/libsolidity/ASTRecoveryTests\|test/compilationTests/zeppelin/LICENSE" |
+  grep -v -E "test/libsolidity/syntaxTests/comments/unicode_direction_override_1.sol"


A comment saying why stuff like this is disabled always helps when you have to refactor it later.

cameel · 2020-12-15T16:31:10Z

liblangutil/Scanner.cpp

+		case ScannerError::DirectionalOverrideUnderflowInComment: return "Unicode direction override underflow in comment or string literal.";
+		case ScannerError::DirectionalOverrideMismatchInComment: return "Mismatching directional override markers in comment or string literal.";


I just noticed that it's not just the error message that was mentioning only comments:

Suggested change

case ScannerError::DirectionalOverrideUnderflowInComment: return "Unicode direction override underflow in comment or string literal.";

case ScannerError::DirectionalOverrideMismatchInComment: return "Mismatching directional override markers in comment or string literal.";

case ScannerError::DirectionalOverrideUnderflow: return "Unicode direction override underflow in comment or string literal.";

case ScannerError::DirectionalOverrideMismatch: return "Mismatching directional override markers in comment or string literal.";

Changelog.md

cameel · 2020-12-15T16:45:27Z

test/libsolidity/syntaxTests/comments/multiline_unicode_direction_override_7.sol

@@ -0,0 +1,7 @@
+contract C {
+    function f() public pure {
+        /* LRO‭ LRE‪ RLE ‫  PDF‬ RLO‮ PDF ‬ PDF‬


I wanted to point out the FDP and only then realized what's going on. It works :) Lost opportunity for O RLY though :)

cameel · 2020-12-15T17:01:32Z

@axic Personally I'm fine either way. Not including this in 0.8.0 and doing it properly for the next breaking release would be best but I'm also assuming that we really want something against that attack. This PR on its own does not get that deep into Unicode yet and is small so should be easy to revert if we want to change the direction. Most of it are tests actually.

christianparpart force-pushed the unicode-directional-marks branch from db2814c to b43c136 Compare November 18, 2020 13:52

This comment has been minimized.

Sign in to view

stackenbotten reviewed Nov 18, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

stackenbotten reviewed Nov 18, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the unicode-directional-marks branch from b43c136 to 66c8a1d Compare November 20, 2020 12:43

This comment has been minimized.

Sign in to view

stackenbotten reviewed Nov 20, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

stackenbotten reviewed Nov 20, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the unicode-directional-marks branch from 66c8a1d to 9d68f25 Compare November 20, 2020 13:08

This comment has been minimized.

Sign in to view

stackenbotten reviewed Nov 20, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the unicode-directional-marks branch from 9d68f25 to 4b113d1 Compare November 20, 2020 13:16

christianparpart marked this pull request as ready for review November 20, 2020 13:26

christianparpart force-pushed the unicode-directional-marks branch 3 times, most recently from ee33371 to fe19966 Compare November 20, 2020 15:02

christianparpart requested review from leonardoalt and Marenz November 23, 2020 09:56

Marenz suggested changes Nov 23, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the unicode-directional-marks branch from fe19966 to b666df5 Compare November 23, 2020 13:31

cameel reviewed Nov 23, 2020

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

liblangutil/Scanner.h Outdated Show resolved Hide resolved

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

Marenz suggested changes Nov 23, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

cameel reviewed Nov 23, 2020

View reviewed changes

chriseth reviewed Nov 23, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

chriseth reviewed Nov 23, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

axic reviewed Nov 24, 2020

View reviewed changes

liblangutil/Scanner.cpp Show resolved Hide resolved

chriseth reviewed Nov 24, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

christianparpart force-pushed the unicode-directional-marks branch 2 times, most recently from 400f27a to 8f428cf Compare November 25, 2020 13:13

mijovic force-pushed the unicode-directional-marks branch from cce652e to 54e215f Compare December 15, 2020 14:20

mijovic force-pushed the unicode-directional-marks branch from 54e215f to cfee4a3 Compare December 15, 2020 14:44

mijovic force-pushed the unicode-directional-marks branch from cfee4a3 to 89d09cd Compare December 15, 2020 14:47

mijovic force-pushed the unicode-directional-marks branch 2 times, most recently from 8ba0d9f to 01a465c Compare December 15, 2020 15:19

chriseth reviewed Dec 15, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

chriseth reviewed Dec 15, 2020

View reviewed changes

liblangutil/Scanner.cpp Outdated Show resolved Hide resolved

chriseth reviewed Dec 15, 2020

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

chriseth reviewed Dec 15, 2020

View reviewed changes

mijovic force-pushed the unicode-directional-marks branch from 6c844e0 to 4b912aa Compare December 15, 2020 16:03

mijovic force-pushed the unicode-directional-marks branch 2 times, most recently from a743337 to 460ed18 Compare December 15, 2020 16:42

cameel reviewed Dec 15, 2020

View reviewed changes

Scanner: Generate error on inbalanced RLO/LRO/PDF override markers.

381c63e

chriseth force-pushed the unicode-directional-marks branch from 033ff61 to 381c63e Compare December 15, 2020 17:41

chriseth approved these changes Dec 15, 2020

View reviewed changes

chriseth merged commit 1729385 into develop Dec 15, 2020

chriseth deleted the unicode-directional-marks branch December 15, 2020 17:54

mijovic mentioned this pull request Dec 15, 2020

[yul] Allow unicode directional sequences in comments and string literals #10622

Closed

cameel mentioned this pull request Dec 16, 2020

Fix line endings in Unicode diretion override tests #10628

Merged

cameel mentioned this pull request Feb 17, 2021

Re-evaluate unicode tricks in comments #10254

Closed

cameel mentioned this pull request Feb 6, 2023

Disallow isolate Unicode characters in comments and strings #13936

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scanner: Generate error on inbalanced RLO/LRO/PDF override markers. #10326

Scanner: Generate error on inbalanced RLO/LRO/PDF override markers. #10326

christianparpart commented Nov 18, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cameel left a comment

cameel left a comment

stackenbotten commented Nov 25, 2020

stackenbotten commented Dec 15, 2020

stackenbotten commented Dec 15, 2020

stackenbotten commented Dec 15, 2020

chriseth Dec 15, 2020

mijovic Dec 15, 2020

axic commented Dec 15, 2020

mijovic commented Dec 15, 2020

cameel commented Dec 15, 2020

axic commented Dec 15, 2020

cameel left a comment

cameel Dec 15, 2020

cameel Dec 15, 2020

cameel Dec 15, 2020

cameel Dec 15, 2020

cameel commented Dec 15, 2020 •

edited

Loading

	pair<string_view, int>{"\xE2\x80\xAC", -1} // PDF - Pop Directional Formatting
	pair<string_view, int>{"\xE2\x80\xAC", -1} // U+202C (PDF - Pop Directional Formatting)

		case ScannerError::DirectionalOverrideUnderflowInComment: return "Unicode direction override underflow in comment or string literal.";
		case ScannerError::DirectionalOverrideMismatchInComment: return "Mismatching directional override markers in comment or string literal.";

Scanner: Generate error on inbalanced RLO/LRO/PDF override markers. #10326

Scanner: Generate error on inbalanced RLO/LRO/PDF override markers. #10326

Conversation

christianparpart commented Nov 18, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cameel left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

stackenbotten commented Nov 25, 2020

stackenbotten commented Dec 15, 2020

stackenbotten commented Dec 15, 2020

stackenbotten commented Dec 15, 2020

chriseth Dec 15, 2020

Choose a reason for hiding this comment

mijovic Dec 15, 2020

Choose a reason for hiding this comment

axic commented Dec 15, 2020

mijovic commented Dec 15, 2020

cameel commented Dec 15, 2020

axic commented Dec 15, 2020

cameel left a comment

Choose a reason for hiding this comment

cameel Dec 15, 2020

Choose a reason for hiding this comment

cameel Dec 15, 2020

Choose a reason for hiding this comment

cameel Dec 15, 2020

Choose a reason for hiding this comment

cameel Dec 15, 2020

Choose a reason for hiding this comment

cameel commented Dec 15, 2020 • edited Loading

christianparpart commented Nov 18, 2020 •

edited

Loading

cameel commented Dec 15, 2020 •

edited

Loading