Skip to content

C++: Positively phrased sanitizer in cpp/non-constant-format #12003

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

MathiasVP
Copy link
Contributor

The sanitizer in the cpp/non-constant-format query was very fragile: it's supposed to look at the type of the node, and mark it as a sanitizer if the type reveals that the node could not possible hold a string.

This was easy to do before we introduced indirect dataflow nodes: we could just mark all non-pointer nodes as sanitizers because we were tracking a value of type char*. However, when we track indirect nodes as well, those nodes can have type char (since we're tracking the indirection of the char).

So when we introduced indirect nodes, we modified the sanitizer to not sanitize indirect nodes (for instance, by saying that node.asIndirectExpr() shouldn't exist). However, this relies on all indirect nodes having a result for asIndirectExpr() which doesn't have to be true (since plenty of dataflow nodes do not map to an expression).

Instead, this PR rephrases the sanitizer to only sanitize nodes for which node.asExpr() holds. This predicate never has a result for indirect nodes, so the effect should be the same, but it won't depend on each indirect dataflow node having a result for asIndirectExpr().

…o longer incorrectly sanitize indirect nodes for which there is no result for 'asIndirectExpr'.
@MathiasVP MathiasVP added the C++ label Jan 27, 2023
@MathiasVP MathiasVP requested a review from a team as a code owner January 27, 2023 10:15
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Jan 27, 2023
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if DCA is happy.

@MathiasVP
Copy link
Contributor Author

DCA showed nothing exciting (🎉 I guess!)

@MathiasVP MathiasVP merged commit e48c93a into github:mathiasvp/replace-ast-with-ir-use-usedataflow Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants