Sitelet https://web.archive.org/web/20260615194238/https://github.com/github/codeql/pull/9175
Skip to content

Ruby: stop considering post-update nodes to be local source nodes#9175

Closed
nickrolfe wants to merge 7 commits into
mainfrom
nickrolfe/exclude_post_update_nodes
Closed

Ruby: stop considering post-update nodes to be local source nodes#9175
nickrolfe wants to merge 7 commits into
mainfrom
nickrolfe/exclude_post_update_nodes

Conversation

@nickrolfe

@nickrolfe nickrolfe commented May 16, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added the Ruby label May 16, 2022
@nickrolfe

nickrolfe commented May 16, 2022

Copy link
Copy Markdown
Contributor Author

Hmm, I may have got this wrong. On the one hand, dataflow queries on rapid7/metasploit-framework now complete in a reasonable time, but on the other hand, this doesn't look great:

Tuple counts for DataFlowPrivate::Cached::isLocalSourceNode#462ff392#f#antijoin_rhs@46f66bui:
        1463003991      ~0%    {2} r1 = JOIN boundedFastTC(DataFlowPrivate::Cached::localFlowStepTypeTracker#462ff392#ff_10#higher_order_body,DataFlowPrivate::Cached::TNode#462ff392#f) WITH DataFlowPrivate::Cached::TNode#462ff392#f ON FIRST 1 OUTPUT Lhs.1, Rhs.0
                           
            369909      ~3%    {1} r2 = JOIN r1 WITH DataFlowPrivate::Cached::entrySsaDefinition#462ff392#f ON FIRST 1 OUTPUT Lhs.1
                           
         731612184  ~88927%    {1} r3 = JOIN r1 WITH DataFlowPrivate::Cached::TExprNode#462ff392#ff_1#join_rhs ON FIRST 1 OUTPUT Lhs.1
                           
         731982093  ~81105%    {1} r4 = r2 UNION r3
                               return r4

@nickrolfe nickrolfe marked this pull request as ready for review May 16, 2022 19:39
@nickrolfe nickrolfe requested a review from a team as a code owner May 16, 2022 19:39
hvitved
hvitved reviewed May 17, 2022
View reviewed changes
Comment thread ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll Outdated
Comment thread ruby/ql/src/queries/security/cwe-116/IncompleteSanitization.ql Outdated
@nickrolfe nickrolfe added the no-change-note-required This PR does not need a change note label May 17, 2022
@nickrolfe

nickrolfe commented May 17, 2022

Copy link
Copy Markdown
Contributor Author

I still see the massive tuple count in the antijoin I posted above.

hvitved
hvitved reviewed May 18, 2022
View reviewed changes
Comment thread ruby/ql/src/queries/security/cwe-116/IncompleteSanitization.ql Outdated
@hvitved
Ruby: Eliminate bad isLocalSourceNode antijoin
d5667bd 
Gets rid of
```
Tuple counts for DataFlowPrivate::Cached::isLocalSourceNode#462ff392#f#antijoin_rhs@dd2f927s:
        20905019     ~3%    {2} r1 = JOIN DataFlowPrivate::Cached::TExprNode#462ff392#ff_1#higher_order_body WITH boundedFastTC(DataFlowPrivate::Cached::localFlowStepTypeTracker#462ff392#ff_10#higher_order_body,DataFlowPrivate::Cached::TExprNode#462ff392#ff_1#higher_order_body) ON FIRST 1 OUTPUT Rhs.1, Lhs.0

        10420128  ~1496%    {1} r2 = JOIN r1 WITH DataFlowPrivate::Cached::TExprNode#462ff392#ff_1#higher_order_body ON FIRST 1 OUTPUT Lhs.1

          480918     ~8%    {1} r3 = JOIN r1 WITH DataFlowPrivate::Cached::entrySsaDefinition#462ff392#f ON FIRST 1 OUTPUT Lhs.1

        10901046  ~1218%    {1} r4 = r2 UNION r3
                            return r4
```
@hvitved

hvitved commented May 18, 2022

Copy link
Copy Markdown
Contributor

I still see the massive tuple count in the antijoin I posted above.

I have pushed a fix for the antijoin.

@nickrolfe @hvitved
Update ruby/ql/src/queries/security/cwe-116/IncompleteSanitization.ql
73b3472 
Ruby: simplify localFlow call

Co-authored-by: Tom Hvitved <hvitved@github.com>
@hvitved hvitved mentioned this pull request May 23, 2022
Merged
@hmac

hmac commented May 26, 2022

Copy link
Copy Markdown
Contributor

@nickrolfe it sounds like Tom was happy with this change. I don't fully understand it but the test changes look plausible. Do you think after a rebase and DCA run we could merge it?

@nickrolfe

nickrolfe commented May 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@hmac the test changes I committed aren't the only ones – the n instanceof ExprNode change suggested by Tom introduced more test failures, and I haven't managed to look into them yet, but my gut instinct was that something got broken.

@hmac

hmac commented May 26, 2022

Copy link
Copy Markdown
Contributor

Ah OK - I was just checking in case this was something that was waiting on a final 👍 from someone.

@hvitved hvitved mentioned this pull request Aug 17, 2022
Merged
@nickrolfe nickrolfe closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hvitved hvitved hvitved left review comments

Assignees

No one assigned

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.

3 participants

@nickrolfe @hvitved @hmac