C++: Add implicit destructors for named variables to the IR #15506

rdmarsh2 · 2024-02-01T19:45:10Z

No description provided.

MathiasVP

Some initial comments. So far I've only looked at some of the .expected files. I think I'll delay looking at too many of the .qll changes until the conflicts with main has been resolved.

I think some of the consistency issues I've noted here would also be revealed by running the cpp/ql/test/library-tests/ir/ tests

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

MathiasVP · 2024-02-02T14:35:35Z

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

+# 1244|     r1244_2(glval<String>)   = VariableAddress[c]              : 
+# 1244|     r1244_3(glval<unknown>)  = FunctionAddress[~String]        : 
+# 1244|     v1244_4(void)            = Call[~String]                   : func:r1244_3, this:r1244_2
+# 1244|     mu1244_5(unknown)        = ^CallSideEffect                 : ~m?
+# 1244|     v1244_6(void)            = ^IndirectReadSideEffect[-1]     : &:r1244_2, ~m?
+# 1244|     mu1244_7(String)         = ^IndirectMayWriteSideEffect[-1] : &:r1244_2
+# 1244|     r1244_8(glval<String>)   = VariableAddress[b]              : 
+# 1244|     r1244_9(glval<unknown>)  = FunctionAddress[~String]        : 
+# 1244|     v1244_10(void)           = Call[~String]                   : func:r1244_9, this:r1244_8
+# 1244|     mu1244_11(unknown)       = ^CallSideEffect                 : ~m?
+# 1244|     v1244_12(void)           = ^IndirectReadSideEffect[-1]     : &:r1244_8, ~m?
+# 1244|     mu1244_13(String)        = ^IndirectMayWriteSideEffect[-1] : &:r1244_8
+# 1244|     r1244_14(glval<String>)  = VariableAddress[a]              : 
+# 1244|     r1244_15(glval<unknown>) = FunctionAddress[~String]        : 
+# 1244|     v1244_16(void)           = Call[~String]                   : func:r1244_15, this:r1244_14
+# 1244|     mu1244_17(unknown)       = ^CallSideEffect                 : ~m?
+# 1244|     v1244_18(void)           = ^IndirectReadSideEffect[-1]     : &:r1244_14, ~m?
+# 1244|     mu1244_19(String)        = ^IndirectMayWriteSideEffect[-1] : &:r1244_14


I don't think this is correct. a, b and c are all static locals. So their destructors should not be called at the end of the scope. Ideally, we should have some kind of new IRFunction that calls these destructors at the end of main, but let's leave that aside for now.

As a start, I think we should simply not emit these destructors for now.

Oh, good point. I missed that those were static when I was looking over the results.

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    result = TranslatedVariableInitialization.super.getChildInternal(id)
+  }
+
+  final override Instruction getChildSuccessorInternal(TranslatedElement elem, EdgeKind kind) {


cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

+    result = TranslatedVariableInitialization.super.getChildInternal(id)
+  }
+
+  final override Instruction getChildSuccessorInternal(TranslatedElement elem, EdgeKind kind) {


cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/IRConstruction.qll

@@ -11,6 +11,7 @@
 private import TranslatedCondition
 private import TranslatedElement
 private import TranslatedExpr
+private import TranslatedCall


rdmarsh2 · 2024-02-02T19:58:58Z

Force-pushed to fix a merge conflict with #15461 and remove some commits from #15318

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/InstructionTag.qll

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedDeclarationEntry.qll

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

This avoids an issue with duplicated qualifiers that was causing broken control flow

MathiasVP · 2024-02-08T11:52:51Z

This looks great, Robert! The unaliased SSA failures are still a mystery to me, but other than excluding destructor of static locals this is looking really good 🎉

The DCA run you started also looks promising. I took the liberty of starting a run that also run the IR consistency queries to make sure we're not adding too many new inconsistencies. Hopefully, the new inconsistencies are simply because we're generating more IR 🤞

rdmarsh2 · 2024-02-09T18:49:58Z

Assuming you're referring to the points_to test, I think what's happening there is that the destructor calls are leading to all the variables in that test escaping, which then means their MemoryLocations become part of AllAliasedMemory, so the results for them get filtered out.

MathiasVP · 2024-02-12T10:59:25Z

Assuming you're referring to the points_to test, I think what's happening there is that the destructor calls are leading to all the variables in that test escaping, which then means their MemoryLocations become part of AllAliasedMemory, so the results for them get filtered out.

That makes sense. It sounds like we now need a side-effect/alias model for the destructor of a smart pointer, then. How about we add something like the following: https://gist.github.com/MathiasVP/a681b702be4ab46d45a9adaf034352c9 ?

cpp/ql/test/library-tests/ir/points_to/points_to.expected

cpp/ql/test/library-tests/ir/ir/raw_ir.expected

MathiasVP · 2024-02-12T11:02:44Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedInitialization.qll

@@ -1009,6 +1067,8 @@ class TranslatedConstructorBareInit extends TranslatedElement, TTranslatedConstr
    result = this.getParent().getChildSuccessor(this, kind)
  }

+  override Instruction getALastInstructionInternal() { none() } // FIXME: does this need to be filled in?


Here's a FIXME that we haven't resolved yet. What's the status of this one?

That class only exists to handle an (old?) extractor bug. I don't think it can ever have a destructor attached, and neither can the init list it's part of, so I think it's fine for getALastChild to have no results.

Sounds good. Should we delete the FIXME comment then? And maybe convert it to an internal issue?

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll

MathiasVP · 2024-02-12T11:08:12Z

Also a question: Do we not need to do anything in this PR with the destruction of static locals now that we know that they aren't handled properly in the extractor?

MathiasVP

I think we're almost good to go! I'll run a final DCA on this since there's been quite a lot of changes since the last run. After that, I think we should eyeball the new results to make sure they're destructor-related, but then I think we can merge this 🎉

MathiasVP · 2024-02-13T09:28:50Z

cpp/ql/lib/semmle/code/cpp/models/implementations/SmartPointer.qll

+    // TODO: Check if this is join ordered correctly.
+    result.getDeclaringType() = declaringType.getBaseType()


Did you check this? Because I was kinda hoping that you would 😂 I think we should just start a fresh DCA run on this PR, and if nothing stands out here we should just delete the TODO comment.

MathiasVP · 2024-02-13T09:31:31Z

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedInitialization.qll

@@ -1009,6 +1067,8 @@ class TranslatedConstructorBareInit extends TranslatedElement, TTranslatedConstr
    result = this.getParent().getChildSuccessor(this, kind)
  }

+  override Instruction getALastInstructionInternal() { none() } // FIXME: does this need to be filled in?


Sounds good. Should we delete the FIXME comment then? And maybe convert it to an internal issue?

MathiasVP · 2024-02-13T12:00:57Z

I think we're almost good to go! I'll run a final DCA on this since there's been quite a lot of changes since the last run. After that, I think we should eyeball the new results to make sure they're destructor-related, but then I think we can merge this 🎉

DCA came back:

No performance problems 🎉 So I think we should just delete the TODO that crept into f791b0e
I think we should look at a small subset of the query results and confirm that these are related to the introduction of destructors and not some new bug in IR construction

geoffw0 · 2024-02-14T16:50:10Z

I think we should look at a small subset of the query results and confirm that these are related to the introduction of destructors and not some new bug in IR construction

I've just had a look at a couple of the Samate result changes in the above DCA run:

new result for cpp/path-injection on dsp-testing__samate-c-cpp: C/testcases/CWE23_Relative_Path_Traversal/s01/CWE23_Relative_Path_Traversal__char_connect_socket_ifstream_83_bad.cpp:124:32:124:36
- this is a result in a "bad" case and appears to be a correct result.
- this involves a class with a constructor that calls recv (the source) and writes the result into a member variable data; then the destructor of the same classes accesses data (at the sink).
- the test creates an object of this class in a local variable. Presumably we were missing the fact that the destructor was called at all prior to this PR. 🎉
new result for cpp/tainted-format-string on dsp-testing__samate-c-cpp: C/testcases/CWE789_Uncontrolled_Mem_Alloc/s01/CWE789_Uncontrolled_Mem_Alloc__new_char_connect_socket_83_bad.cpp:118:24:118:38
- this is a result in a "bad" case and appears to be a correct result.
- very similar to the previous case; the sink is different. 👍

So these both look good, and I'm pretty confident all of the other Samate test changes are explained by the same pattern (they appear to be all for cases labelled '83', which means they'll follow the same constructor-destructor pattern).

There are a handful of results on other projects besides Samate - someone should review a few of these as well.

rdmarsh2 · 2024-02-16T05:43:56Z

Looked into a couple of those today (had a horrible time getting them to evaluate without hitting some sort of cache...) and looks like they're false positives related to an issue with successors of return statements. Shoudl have a fix tomorrow.

MathiasVP

The new changes makes sense, and it looks like this removed a bunch of seemingly DCA results. I did verify that the use-after-free results that were removed were instances of the successor issue that you fixed in 2494b7d.

I think I spotted one bug in your fix commit. I tried to run this example after this fix:

struct String
{
  String();
  ~String();
};

void VoidFunc();

void IfReturnDestructors(bool b) {
  String s;
  return VoidFunc();
}

but now I'm getting a bunch of duplication and an unexplained loop: https://gist.github.com/MathiasVP/7afa7176a7a4bf4750c8579a04855ec4

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll

Fixes conflicts in C++ IR tests and Stmt.qll

jketema · 2024-02-22T10:44:32Z

@rdmarsh2 it seems your cli is somewhat out-of-date, as your latest changes to the PrintAST output revert some stuff that was fixed.

I would also be sensible to re-run DCA on this, given that things were fixed.

MathiasVP

One small nit, but otherwise I'm happy with this as soon as we have a happy DCA run! (Although, the .expected files obviously need to be modified as per Jeroen's comment)

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll

cpp/ql/test/library-tests/ir/ir/aliased_ir.expected

MathiasVP

I think we're good to go! 2 remaining things:

We need to add a change note. I suggest making a library change note of category minorAnalysis.
We should probably start a DCA run to test the impact of the last IR fix (Edit: I took the liberty of starting this one myself. I hope that's okay!)

rdmarsh2 · 2024-02-26T20:12:57Z

There's one new alert in the DCA that looks like a false positive... It's a use-after-free involving a delete in a loop, but there's an unconditional break following the free. I'm not sure if it's a pre-existing CFG issue that's now being exposed or a new CFG issue.

MathiasVP · 2024-02-27T12:35:12Z

There's one new alert in the DCA that looks like a false positive... It's a use-after-free involving a delete in a loop, but there's an unconditional break following the free. I'm not sure if it's a pre-existing CFG issue that's now being exposed or a new CFG issue.

I'm not too concerned about one new FP result amidst all the new TPs we're seeing on Samate (which Geoffrey looked at earlier). So if you investigated this one and concluded that it's a FP then I think this is good enough, and we should create an issue for why this happens as a follow-up.

MathiasVP

LGTM!

github-actions bot added the C++ label Feb 1, 2024

MathiasVP reviewed Feb 2, 2024

View reviewed changes

rdmarsh2 added 7 commits February 2, 2024 17:05

C++: generate instructions for destructor calls in IR

47720e0

C++: Add getLastInstruction to IR generation

3a404ce

C++: Add implicit named destructosrs to the IR CFG

85d1d07

C++: custom destructor handling for for loops

820f4a5

C++: test for destructors in range-based for

4513fd1

C++: test for declarations in if statement

984c7ab

C++: Test for destructors in declaration as if condition

2d010f6

rdmarsh2 force-pushed the rdmarsh2/cpp/ir-synthetic-destructors branch from a341acf to 2d010f6 Compare February 2, 2024 17:40

github-advanced-security bot found potential problems Feb 2, 2024

View reviewed changes

MathiasVP reviewed Feb 5, 2024

View reviewed changes

rdmarsh2 added 4 commits February 5, 2024 18:41

C++: add test for constructors in C++17 decl-in-if

bbabf1d

C++: QLDoc and naming updates for implicit destructors in IR

8013c2a

C++: suppress destructor calls on delete in IR generation

40e06b7

This avoids an issue with duplicated qualifiers that was causing broken control flow

C++: Update IR test expectations for named destructors

5653c3f

MathiasVP mentioned this pull request Feb 7, 2024

C++: Fix IR generation when ConditionDeclExpr does not have an immediate VariableAccess #15539

Merged

Merge branch 'main' into rdmarsh2/cpp/ir-synthetic-destructors

1749661

rdmarsh2 added 2 commits February 9, 2024 22:32

C++: Accept test changes

1b571f8

C++: Autoformat for named destructors in IR

d1160f8

MathiasVP reviewed Feb 12, 2024

View reviewed changes

rdmarsh2 added 4 commits February 12, 2024 23:55

C++: tests for destructors after a while-loop condition

bac7e46

C++: fix for destructor of while-loop condition

b94c4a6

C++: test for multiple for loop variables with destructors

6663420

C++: Fix for multiple for-loop variables with destructors

7d8872b

C++: autoformat

b9785ea

MathiasVP reviewed Feb 13, 2024

View reviewed changes

rdmarsh2 added 2 commits February 13, 2024 15:34

C++: delete some FIXMEs that turned out fine

128bc99

Merge branch 'main' into rdmarsh2/cpp/ir-synthetic-destructors

7e23ccd

rdmarsh2 added 2 commits February 16, 2024 17:55

C++: test for return in if

2c8ed64

C++: fix for IR CFG problem with return in if

2494b7d

MathiasVP reviewed Feb 19, 2024

View reviewed changes

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll Outdated Show resolved Hide resolved

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll Outdated Show resolved Hide resolved

rdmarsh2 added 3 commits February 21, 2024 20:35

C++: fix incorrect use of getChildInternal

e0c7849

C++: refactor TranslatedReturnStmt

66743fb

Merge branch 'main' into rdmarsh2/cpp/ir-synthetic-destructors

875ab74

Fixes conflicts in C++ IR tests and Stmt.qll

MathiasVP reviewed Feb 22, 2024

View reviewed changes

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll Outdated Show resolved Hide resolved

cpp/ql/test/library-tests/ir/ir/aliased_ir.expected Outdated Show resolved Hide resolved

rdmarsh2 added 5 commits February 22, 2024 16:44

C++: accept test changes from extractor fixes

ebe6ee5

C++: move handlesDestructorsExplicitly up to TranslatedReturnStmt

942a4ed

Merge branch 'main' into rdmarsh2/cpp/ir-synthetic-destructors

6f7f68f

C++: fix for duplicated parent of ReturnVoid statements

dd97584

C++: autoformat

da5e3d6

MathiasVP reviewed Feb 23, 2024

View reviewed changes

github-actions bot added the documentation label Feb 26, 2024

C++: Change note for IR named destructors.

a513598

rdmarsh2 force-pushed the rdmarsh2/cpp/ir-synthetic-destructors branch from 2454252 to a513598 Compare February 26, 2024 19:42

MathiasVP mentioned this pull request Feb 27, 2024

C++: IR construction for VacuousDestructorCalls #15735

Merged

MathiasVP approved these changes Feb 27, 2024

View reviewed changes

MathiasVP merged commit f7b2de8 into github:main Feb 27, 2024
15 checks passed

rdmarsh2 mentioned this pull request Feb 29, 2024

C++: Output destructors of temporary objects #15770

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++: Add implicit destructors for named variables to the IR #15506

C++: Add implicit destructors for named variables to the IR #15506

rdmarsh2 commented Feb 1, 2024

MathiasVP left a comment •

edited

Loading

MathiasVP Feb 2, 2024

rdmarsh2 Feb 2, 2024

rdmarsh2 commented Feb 2, 2024

MathiasVP commented Feb 8, 2024 •

edited

Loading

rdmarsh2 commented Feb 9, 2024

MathiasVP commented Feb 12, 2024

MathiasVP Feb 12, 2024 •

edited

Loading

rdmarsh2 Feb 13, 2024

MathiasVP Feb 13, 2024

MathiasVP commented Feb 12, 2024

MathiasVP left a comment

MathiasVP Feb 13, 2024

MathiasVP Feb 13, 2024

MathiasVP commented Feb 13, 2024

geoffw0 commented Feb 14, 2024

rdmarsh2 commented Feb 16, 2024

MathiasVP left a comment •

edited

Loading

jketema commented Feb 22, 2024

MathiasVP left a comment

MathiasVP left a comment •

edited

Loading

rdmarsh2 commented Feb 26, 2024

MathiasVP commented Feb 27, 2024

MathiasVP left a comment

		// TODO: Check if this is join ordered correctly.
		result.getDeclaringType() = declaringType.getBaseType()

C++: Add implicit destructors for named variables to the IR #15506

C++: Add implicit destructors for named variables to the IR #15506

Conversation

rdmarsh2 commented Feb 1, 2024

MathiasVP left a comment • edited Loading

Choose a reason for hiding this comment

MathiasVP Feb 2, 2024

Choose a reason for hiding this comment

rdmarsh2 Feb 2, 2024

Choose a reason for hiding this comment

rdmarsh2 commented Feb 2, 2024

MathiasVP commented Feb 8, 2024 • edited Loading

rdmarsh2 commented Feb 9, 2024

MathiasVP commented Feb 12, 2024

MathiasVP Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

rdmarsh2 Feb 13, 2024

Choose a reason for hiding this comment

MathiasVP Feb 13, 2024

Choose a reason for hiding this comment

MathiasVP commented Feb 12, 2024

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP Feb 13, 2024

Choose a reason for hiding this comment

MathiasVP Feb 13, 2024

Choose a reason for hiding this comment

MathiasVP commented Feb 13, 2024

geoffw0 commented Feb 14, 2024

rdmarsh2 commented Feb 16, 2024

MathiasVP left a comment • edited Loading

Choose a reason for hiding this comment

jketema commented Feb 22, 2024

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment • edited Loading

Choose a reason for hiding this comment

rdmarsh2 commented Feb 26, 2024

MathiasVP commented Feb 27, 2024

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment •

edited

Loading

MathiasVP commented Feb 8, 2024 •

edited

Loading

MathiasVP Feb 12, 2024 •

edited

Loading

MathiasVP left a comment •

edited

Loading

MathiasVP left a comment •

edited

Loading