C++: Map operand nodes that are only used once onto the related instruction node #12004

jketema · 2023-01-27T10:27:40Z

Also revert the changes to default taint tracking and TaintedAllocationSize.ql as these are no longer needed with the above mapping.

jketema · 2023-01-27T10:32:28Z

...curity/CWE/CWE-129/semmle/ImproperArrayIndexValidation/ImproperArrayIndexValidation.expected

-| test1.c:41:11:41:11 | i | test1.c:7:26:7:29 | argv | test1.c:41:11:41:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | argv | a command-line argument |
-| test1.c:41:11:41:11 | i | test1.c:7:26:7:29 | argv indirection | test1.c:41:11:41:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | argv indirection | a command-line argument |
-| test1.c:41:11:41:11 | i | test1.c:7:26:7:29 | argv indirection | test1.c:41:11:41:11 | i | An array indexing expression depends on $@ that might be outside the bounds of the array. | test1.c:7:26:7:29 | argv indirection | a command-line argument |


Technically this is a regression. However, this is also an FN on main, so I'm not immediately worried by this. Especially since the query actually digs into InstructionNodes.

MathiasVP

Awesome work. LGTM if DCA is happy!

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

MathiasVP · 2023-01-27T10:47:28Z

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternalsCommon.qll

+   */
+  cached
+  Instruction getIRRepresentationOfOperand(Operand operand) {
+    operand = unique( | | result.getAUse())


I just remembered that, while getAUse is probably not wrong to use here, we might actually be able to reduce the number of nodes even more if we replace result.getAUse() with getAUse(result) (which is a predicate that only finds the uses that are not ignored for dataflow purposes). This will likely result in more unique operands 🎉.

We don't need to do that in this PR (since this PR is already doing wonderful usability improvements as-is), and I think a similar fix could be applied in the indirection case (where I think I made the exact same mistake).

I'll try to do a follow-up PR.

Interestingly enough trying this gives me consistency errors in the dataflow tests.

Oh, that's weird. Which ones are failing?

They all seem to be related to constructors.

Multiple toStrings does sound like something we'd be able to fix, though 😄. That's probably also the reason for the new localFlowIsLocal inconsistencies (since multiple toStrings probably means that a inconsistency is reported multiple times).

Multiple toStrings often happens because a node belongs to several QL classes that each define a toString. Based on this, I think the way forward would be to use getAQlClass to determine the overlapping node classes for some particular entity that belong to multiple classes that both define a toString. We can then figure out if we can merge those two classes in certain cases, or exclude the toString result when they overlap.

If this turns out to be too complicated, feel free to create an issue for it and leave it as future work.

Definitely seems fixable, and yeah need to determine what the overlapping classes are.

I'll think I create an issue for it in any case, as this in not the top priority item and not needed to get the feature branch merged. Will revisit once I've re-worked the feature branch to be opt-in.

MathiasVP · 2023-01-27T15:58:34Z

I just realized that we might now have some local flow steps from Instructions to Operand (i.e., the ones from the simpleOperandLocalFlowStep predicate) that are now the identity relation. They should probably be excluded from the predicate.

jketema · 2023-01-27T16:00:33Z

I'll have a look on Monday.

MathiasVP · 2023-01-30T10:27:15Z

Another thing I thought of while I couldn't sleep last night (😭): Since Instructions are now also sometimes defined by the TSingleUseOperandNode0 shouldn't we also modify the body of the TInstructionNode0 branch to exclude the instructions which has a single use?

jketema · 2023-01-31T10:54:52Z

I just realized that we might now have some local flow steps from Instructions to Operand (i.e., the ones from the simpleOperandLocalFlowStep predicate) that are now the identity relation. They should probably be excluded from the predicate.

Do you mean something like:

   private predicate simpleOperandLocalFlowStep(Instruction iFrom, Operand opTo) {
     not opTo instanceof MemoryOperand and
+    not iFrom = Ssa::getIRRepresentationOfOperand(opTo) and
     opTo.getDef() = iFrom
   }

MathiasVP · 2023-01-31T11:01:26Z

Yeah, something along those lines.

jketema · 2023-01-31T11:44:13Z

Mmm, I'm now missing a bunch of results. They're all related to argv sources.

MathiasVP · 2023-01-31T11:49:33Z

Ah, it might be because of my previous comment:

Another thing I thought of while I couldn't sleep last night (😭): Since Instructions are now also sometimes defined by the TSingleUseOperandNode0 shouldn't we also modify the body of the TInstructionNode0 branch to exclude the instructions which has a single use?

since otherwise an Instruction could satisfy both disjuncts in the charpred for InstructionNode:

node.(InstructionNode0).getInstruction() = instr
or
exists(Operand op |
  instr = Ssa::getIRRepresentationOfOperand(op) and
  node.(SingleUseOperandNode0).getOperand() = op
)

jketema · 2023-01-31T13:12:19Z

Ah, it might be because of my previous comment

With:

     TInstructionNode0(Instruction i) {
       not Ssa::ignoreInstruction(i) and
+      not exists(Operand op | i = Ssa::getIRRepresentationOfOperand(op)) and
       // We exclude `void`-typed instructions because they cannot contain data

even more results go missing.

jketema · 2023-02-02T09:25:31Z

Converting this to draft, while further investigating the problems with this PR.

jketema · 2023-02-06T14:02:04Z

Mmm, I'm now missing a bunch of results. They're all related to argv sources.

I think the issue is that simpleOperandLocalFlowStep is used in two different ways. Once in simpleLocalFlowStep and once in indirectionInstructionFlow. Restricting the steps from instructions to operands probably only applies in the former case? If so, the change from #12004 (comment) either needs to move to simpleLocalFlowStep, or simpleOperandLocalFlowStep needs to know (boolean argument?) whether we're looking at the direct or indirect case.

MathiasVP · 2023-02-06T14:05:13Z

I think the issue is that simpleOperandLocalFlowStep is used in two different ways. Once in simpleLocalFlowStep and once in indirectionInstructionFlow. Restricting the steps from instructions to operands probably only applies in the former case? If so, the change from #12004 (comment) either needs to move to simpleLocalFlowStep, or simpleOperandLocalFlowStep needs to know (boolean argument?) whether we're looking at the direct or indirect case.

Hm, yes. That's a good point! Either of those solutions sounds good to me.

jketema · 2023-02-06T16:28:23Z

This now implements the restriction to simpleOperandLocalFlowStep. Test changes look a whole lot nicer than before.

Looking into the IPA restriction now. That still loses test results, so needs more digging.

…uction node

jketema · 2023-02-07T16:50:43Z

Performance looks much better in the latest DCA run than in previous ones. Just need to have a look at the alert changes.

jketema · 2023-02-09T10:04:48Z

Also revert the changes to default taint tracking and TaintedAllocationSize.ql as these are no longer needed with the above mapping.

I've removed this from the PR. It introduced a few new alerts, which seemed to be FPs. I think the conclusion is that that mapping instructions in operands to the same nodes is a usability improvement, but there's of course an edge case with multiple uses where more care might be needed by a query developer.

jketema · 2023-02-09T10:08:30Z

Relevant DCA experiment is not the ultimate one linked from, but the penultimate one (ultimate one was with default taint tracking and TaintedAllocationSize.ql change). As discussed internally, the alert changes for cpp/path-injection look reasonable. The disappearing alerts I've looked at (all PHP ones, and 3 of the 4 systemd ones) are due to the sanitizer now kicking in, where it wasn't before. I spot checked the new git alerts, and they look reasonable.

jketema · 2023-02-09T10:10:09Z

@MathiasVP I re-requested a review, as this has changed substantially from the version you approved.

MathiasVP

The changes LGTM! I think we should do the suggested refactoring to save ourselves some annoying debugging in the future 😂.

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

MathiasVP

LGTM once DCA is happy!

jketema · 2023-02-09T16:18:33Z

DCA seems unchanged.

github-actions bot added the C++ label Jan 27, 2023

jketema force-pushed the single-use branch from 9142ea5 to deeaf70 Compare January 27, 2023 10:29

jketema commented Jan 27, 2023

View reviewed changes

jketema marked this pull request as ready for review January 27, 2023 10:32

jketema requested a review from a team as a code owner January 27, 2023 10:32

jketema added the no-change-note-required This PR does not need a change note label Jan 27, 2023

MathiasVP approved these changes Jan 27, 2023

View reviewed changes

jketema marked this pull request as draft February 2, 2023 09:24

jketema force-pushed the single-use branch from e163a1e to 2737961 Compare February 2, 2023 09:25

jketema force-pushed the single-use branch from 2737961 to 96918a9 Compare February 6, 2023 16:03

C++: Map operand nodes that are only used once onto the related instr…

20ce4cd

…uction node

jketema force-pushed the single-use branch from 96918a9 to 20ce4cd Compare February 7, 2023 13:18

jketema force-pushed the single-use branch from 6fa9533 to 20ce4cd Compare February 9, 2023 10:02

jketema requested a review from MathiasVP February 9, 2023 10:09

jketema marked this pull request as ready for review February 9, 2023 10:09

MathiasVP reviewed Feb 9, 2023

View reviewed changes

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll Outdated Show resolved Hide resolved

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll Outdated Show resolved Hide resolved

C++: Address review comments

e4c211d

jketema force-pushed the single-use branch from abfdda9 to e4c211d Compare February 9, 2023 10:58

MathiasVP approved these changes Feb 9, 2023

View reviewed changes

jketema merged commit 9d6098a into github:mathiasvp/replace-ast-with-ir-use-usedataflow Feb 9, 2023

jketema deleted the single-use branch February 9, 2023 16:18

MathiasVP mentioned this pull request Mar 7, 2025

C++: Share indirect dataflow nodes across CopyValue instructions #18955

Merged

C++: Map operand nodes that are only used once onto the related instruction node #12004

C++: Map operand nodes that are only used once onto the related instruction node #12004

Uh oh!

Conversation

jketema commented Jan 27, 2023

Uh oh!

jketema Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Jan 27, 2023

Uh oh!

jketema commented Jan 27, 2023

Uh oh!

MathiasVP commented Jan 30, 2023

Uh oh!

jketema commented Jan 31, 2023

Uh oh!

MathiasVP commented Jan 31, 2023

Uh oh!

jketema commented Jan 31, 2023

Uh oh!

MathiasVP commented Jan 31, 2023

Uh oh!

jketema commented Jan 31, 2023

Uh oh!

jketema commented Feb 2, 2023

Uh oh!

jketema commented Feb 6, 2023

Uh oh!

MathiasVP commented Feb 6, 2023

Uh oh!

jketema commented Feb 6, 2023

Uh oh!

jketema commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jketema commented Feb 9, 2023

Uh oh!

jketema commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jketema commented Feb 9, 2023

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MathiasVP left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jketema commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jketema Jan 27, 2023 •

edited

Loading

jketema commented Feb 7, 2023 •

edited

Loading

jketema commented Feb 9, 2023 •

edited

Loading

MathiasVP left a comment •

edited

Loading