-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Parse] Fix buffer overrun in advanceIfMultilineDelimiter
#85889
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Make sure we don't scan off the end of the buffer, and scan for the delimiter before attempting the lookahead.
|
@swift-ci please test |
|
@swift-ci please SourceKit stress test |
| const char *TmpPtr = CurPtr; | ||
| if (*(TmpPtr - 1) == '"' && | ||
| diagnoseZeroWidthMatchAndAdvance('"', TmpPtr, Diags) && | ||
| diagnoseZeroWidthMatchAndAdvance('"', TmpPtr, Diags)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No check for EndPtr because we guarantee *EndPtr is 0?
But maybe we could early check like:
if (CurPtr + 2 >= EndPtr) return false;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly, e.g delimiterMatches does
while (diagnoseZeroWidthMatchAndAdvance('#', TmpPtr, Diags)) {}I don't mind adding an extra check though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Considering delimiterMatches does that, I don't think extra check is needed.
| const char *&CurPtr, const char *EndPtr, | ||
| DiagnosticEngine *Diags, | ||
| bool IsOpening = false) { | ||
| auto scanDelimiter = [&]() -> const char * { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanDelimiter is used only once. Why not
const char *DelimEnd = nullptr;
{
// CurPtr here points to the character after `"`.
const char *TmpPtr = CurPtr;
if (*(TmpPtr - 1) == '"' &&
diagnoseZeroWidthMatchAndAdvance('"', TmpPtr, Diags) &&
diagnoseZeroWidthMatchAndAdvance('"', TmpPtr, Diags)) {
DelimEnd = TmpPtr;
} else {
return false;
}
}If it's just a matter of the code style, I'm fine with the lambda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah personally I prefer the lambda, shame C++ doesn't have guard statements 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or just like swift-syntax counterpart
// CurPtr here points to the character after `"`.
const char *DelimEnd = CurPtr;
if (*(DelimEnd - 1) == '"' &&
diagnoseZeroWidthMatchAndAdvance('"', DelimEnd, Diags) &&
diagnoseZeroWidthMatchAndAdvance('"', DelimEnd, Diags)) {
// Found the delimiter
} else {
return false;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty if body feels weird to me, I don't mind changing it though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I'm fine with as-is
Make sure we don't scan off the end of the buffer, and scan for the delimiter before attempting the lookahead.
rdar://165774720