The Wayback Machine - http://web.archive.org/web/20260108105532/https://github.com/github/codeql/pull/9407
Skip to content

RB: fix bad CP in the charPred for CipherOperation #9407

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.

Sign up for GitHub

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

Jump to bottom
Merged
erik-krogh merged 1 commit into from
Jun 13, 2022
Merged

RB: fix bad CP in the charPred for CipherOperation #9407

erik-krogh merged 1 commit into from
Jun 13, 2022

Conversation

@erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jun 1, 2022 •
edited
Loading

I'm doing some on-the-side experiments into performance of the Ruby analysis.

One of those experiments caused something to change (inlining?), which caused a bad Cartesian product to materialize.

Here are the tuple counts:

Tuple counts for OpenSSL::CipherOperation#class#74b60a3e#fff@e3d3fb6n:
        96368664  ~10%    {2} r1 = JOIN DataFlowPrivate::Cached::TNode#462ff392#f WITH project#OpenSSL::CipherInstantiation#class#74b60a3e#fff#3 CARTESIAN PRODUCT OUTPUT Rhs.0, Lhs.0
        96368664   ~3%    {3} r2 = JOIN r1 WITH project#OpenSSL::CipherNode#class#74b60a3e#fff ON FIRST 1 OUTPUT Lhs.0, Lhs.0, Lhs.1
                      
         8030722   ~0%    {2} r3 = SCAN DataFlowPrivate::Cached::TNode#462ff392#f OUTPUT 0, In.0
          747315   ~0%    {2} r4 = JOIN r3 WITH DataFlowPublic::CallNode::getArgument#dispred#f0820431#fbf_120#join_rhs ON FIRST 2 OUTPUT Rhs.2, Lhs.1
              51   ~2%    {2} r5 = JOIN r4 WITH DataFlowPublic::CallNode::getMethodName#dispred#f0820431#fb#cpe#1 ON FIRST 1 OUTPUT Lhs.0, Lhs.1
              51   ~0%    {3} r6 = JOIN r5 WITH DataFlowPublic::CallNode::getReceiver#dispred#f0820431#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.1, Lhs.0
               1   ~0%    {3} r7 = JOIN r6 WITH project#OpenSSL::CipherNode#class#74b60a3e#fff ON FIRST 1 OUTPUT Lhs.2, Lhs.0, Lhs.1
                      
        96368665   ~3%    {3} r8 = r2 UNION r7
                          return r8

The top line is a Cartesian product between all CipherInstantiation instances and all TNode instances.

To be clear: The bad Cartesian product doesn't appear to happen on main, but that is only due to some lucky inlining (I think).

The problem is that the input field in CipherOperation is only bound on one of the disjuncts of the charPred.
This resulted in CipherOperation::getAnInput producing every DataFlow::Node exactly when it was supposed to have no results.
And that is obviously wrong.

The solution is to just delete the input field, and move this.getArgument(0) down into getAnInput.

Evaluation was uneventful.

An annoying thing is that this QL-for-QL PR I opened back in January would have caught it: #7669
I could use more volunteers for reviewing QL-for-QL.
Remember: every CodeQL engineer can approve QL-for-QL PRs, even if they are not a code-owner.

@erik-krogh erik-krogh added no-change-note-required This PR does not need a change note Ruby labels Jun 1, 2022
@erik-krogh erik-krogh marked this pull request as ready for review June 2, 2022 07:15
@erik-krogh erik-krogh requested a review from a team as a code owner June 2, 2022 07:15
alexrford
alexrford approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, this was a silly mistake.

@erik-krogh erik-krogh merged commit caf1d45 into github:main Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@alexrford alexrford alexrford approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alexrford alexrford

Labels

no-change-note-required This PR does not need a change note Ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@erik-krogh @alexrford