[PATCH] D41423: [Lex] Avoid out-of-bounds dereference in LexAngledStringLiteral.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai marked 2 inline comments as done.
vsapsai added inline comments.


================
Comment at: clang/lib/Lex/Lexer.cpp:2026
+
+    if (C == 0) {
       NulCharacter = CurPtr-1;
----------------
dexonsmith wrote:
> vsapsai wrote:
> > dexonsmith wrote:
> > > Should this check still be skipped (in an `else if` of the `C == '\\'` check)?
> > I agree it is behaviour change. `NulCharacter` is used to warn if you have null character in the string and I think it is worth warning even if it is preceded by the backslash. Therefore I think this check shouldn't be skipped after `C == '\\'` check. In practice I don't know how you can create a file with null character in its name and use it in inclusion directive.
> Can you add a test for the behaviour change then?
Added clang/test/Lexer/null-character-in-literal.c but struggling with Phabricator to display it as text file, not as binary. Guess those null characters can be confusing for some tools.


================
Comment at: clang/unittests/Lex/LexerTest.cpp:477
 TEST_F(LexerTest, AvoidPastEndOfStringDereference) {
-  std::vector<Token> LexedTokens = Lex("  //  \\\n");
-  EXPECT_TRUE(LexedTokens.empty());
+  EXPECT_TRUE(Lex("  //  \\\n").empty());
+  // rdar://problem/35572754
----------------
dexonsmith wrote:
> vsapsai wrote:
> > dexonsmith wrote:
> > > To minimize the diff, please separate this change into an NFC commit ahead of time.
> > I plan to nominate this fix for inclusion in 6.0.0 release and having one commit will be easier. It is not necessary to include NFC change in 6.0.0 release but it creates discrepancy that increases a chance of merge conflicts.
> > 
> > But I don't have strong opinion, just pointing out potential downsides with merging the change to other branches.
> I think it's worth separating the NFC changes from behaviour changes, even if it means having to cherry-pick extra patches.
OK, done.


https://reviews.llvm.org/D41423