[clang-tidy] RawStringLiteralCheck: isRawStringLiteral doesn't check all erroneous cases

[gmit]

Summary

isRawStringLiteral will read from memory randomly if double quote is not found.

[lebedev.ri]

1. test?
2. please always upload all patches with full context (-U99999)
3. The diff is malformed. RawStringLiteralCheck.cpp is not on the top level of the repo

[gmit]

Sorry, this is my first LLVM commit, so I don't really know the procedure.

There is no test as it cannot be reproduced. The crash caused by this was spotted in one of our customer's crash dumps. The added condition should be obvious as it is tested in the assert just above.

Could you advise how to make a diff you'd like with TortoiseSVN? I don't seem to be able to add additional parameters. I have attached a new one with full path to the file.

[lebedev.ri]

In D58178#1396388, @gmit wrote:

The added condition should be obvious as it is tested in the assert just above.

Well, then the fix is not correct. Because you won't get to that return,
since the assert() just above would have failed already..
This really needs a test.

[gmit]

? I'm sorry, but I disagree.

Assert draws attention in the debug build only.

In the release build asserts are not evaluated at all and the condition needs to be checked in the code that executes (if it makes sense and in this case it does as it causes reading out of string bounds).

I don't have a test as I don't have an input file that causes this behaviour.

[MyDeveloperDay]

Rather than sending a review, probably the reproducible case as to why it crash is more important, it might be better to write up your bug at https://bugs.llvm.org/

I tried to repoduce this and I can't see how I could make a string literal without a double quote, if it still failed with the current code it might suggest the matcher matched something else.. understanding what that is is probably more important, than just masking the failure.

there was a crash D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check which was fixed back in April, are you able to tell us the version/revision of clang-tidy where this occurred

[lebedev.ri]

In D58178#1396435, @gmit wrote:

? I'm sorry, but I disagree.

Assert draws attention in the debug build only.

In the release build asserts are not evaluated at all and the condition needs to be checked in the code that executes (if it makes sense and in this case it does as it causes reading out of string bounds).

What i'm saying is, do you agree that the assert(QuotePos != StringRef::npos); happens before return (QuotePos != StringRef::npos) && .... ?
If you build with asserts enabled, that your original code which caused it to "read from memory randomly"
will trigger that assertion. Even with this fix. So the fix is not correct.

I don't have a test as I don't have an input file that causes this behaviour.

It's quite easy to trim down the source that crashes down to something usable with https://embed.cs.utah.edu/creduce/
The end result won't resemble anything the original code was, no proprietary secrets will be leaked.
Without test case, there is nothing do to here.

[gmit]

In D58178#1396523, @MyDeveloperDay wrote:

I tried to repoduce this and I can't see how I could make a string literal without a double quote, if it still failed with the current code it might suggest the matcher matched something else.. understanding what that is is probably more important, than just masking the failure.

I agree to that. There is not obvious reason why this would happen.

there was a crash D19331: [Clang-tidy] Fix for crash in modernize-raw-string-literal check which was fixed back in April, are you able to tell us the version/revision of clang-tidy where this occurred

The crash happened on Clang 5 and I can confirm fix 19331 was present in the source files that built it.

It's quite easy to trim down the source that crashes down to something usable with https://embed.cs.utah.edu/creduce/
The end result won't resemble anything the original code was, no proprietary secrets will be leaked.
Without test case, there is nothing do to here.

No, it's not that - I simply don't have it. I don't have .dmp either. I only have our fix that was done in our local Clang repository copy two years ago and a call stack.

Anyhow, I agree to dismiss this case. Sorry for wasting your time! Thanks!

[MyDeveloperDay]

Could I ask if you don't want to pursue this review that you Abandon it.

[gmit]

In D58178#1405397, @MyDeveloperDay wrote:

Could I ask if you don't want to pursue this review that you Abandon it.

Sure, I already wanted that, but I don't see any options that would result in closing the issue. Could you, please, point me to it? Tnx!

[MyDeveloperDay]

In D58178#1405430, @gmit wrote:

In D58178#1405397, @MyDeveloperDay wrote:

Could I ask if you don't want to pursue this review that you Abandon it.

Sure, I already wanted that, but I don't see any options that would result in closing the issue. Could you, please, point me to it? Tnx!

Scroll to the bottom of the review to the "Add Action..." combo box, then select "Abandon Revision"

clang-tools-extra/tags/RELEASE_800/rc2/clang-tidy/modernize/RawStringLiteralCheck.cpp

   // Already a raw string literal if R comes before ".
   const size_t QuotePos = Text.find('"');
   assert(QuotePos != StringRef::npos);
-  return (QuotePos > 0) && (Text[QuotePos - 1] == 'R');
+  return (QuotePos != StringRef::npos) && (QuotePos > 0) &&
+         (Text[QuotePos - 1] == 'R');
 }
 
 bool containsEscapedCharacters(const MatchFinder::MatchResult &Result,