CPP: Add query for detecting invalid uses of temporary unique pointers. #15078

alexet · 2023-12-12T16:15:18Z

This adds a new query for finding cases like T* x = functionReturningUniquePtr().get() where a reference to the internals of a temporary unique pointer lasts longer than the unique pointer. This is based of the c_str query and shares some of the code from it.

github-actions · 2023-12-12T16:16:23Z

QHelp previews:

cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEnds.qhelp

Use of unique pointer after lifetime ends

Calling get on a std::unique_ptr object returns a pointer to the underlying allocations. When the std::unique_ptr object is destroyed, the pointer returned by get is no longer valid. If the pointer is used after the std::unique_ptr object is destroyed, then the behavior is undefined.

Recommendation

Ensure that the pointer returned by get does not outlive the underlying std::unique_ptr object.

Example

The following example gets a std::unique_ptr object, and then converts the resulting unique pointer to a pointer using get so that it can be passed to the work function. However, the std::unique_ptr object is destroyed as soon as the call to get returns. This means that work is given a pointer to invalid memory.

#include <memory>
std::unique_ptr<T> getUniquePointer();
void work(const T*);

// BAD: the unique pointer is deallocated when `get` returns. So `work`
// is given a pointer to invalid memory.
void work_with_unique_ptr_bad() {
  const T* combined_string = getUniquePointer().get();
  work(combined_string);
}

The following example fixes the above code by ensuring that the pointer returned by the call to get does not outlive the underlying std::unique_ptr objects. This ensures that the pointer passed to work points to valid memory.

#include <memory>
std::unique_ptr<T> getUniquePointer();
void work(const T*);

// GOOD: the unique pointer outlives the call to `work`. So the pointer
// obtainted from `get` is valid.
void work_with_unique_ptr_good() {
  auto combined_string = getUniquePointer();
  work(combined_string.get());
}

References

MEM50-CPP. Do not access freed memory.
Common Weakness Enumeration: CWE-416.
Common Weakness Enumeration: CWE-664.

MathiasVP

I couldn't help taking a peek at the query, and this looks super good already!

MathiasVP

The code (and docs) LGTM! We might as well do a DCA run for good measure while we wait for the docs team to review the documentation.

Oh, we do need a change note, though

cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEnds.ql

cpp/ql/src/change-notes/2023-12-12-use-of-unique-pointer-after-lifetime-ends.md

Fix spelling in query id Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

alexet · 2023-12-12T17:24:44Z

cpp/ql/src/Security/CWE/CWE-416/Temporaries.qll

+ * Holds if the value of `e` outlives the enclosing full expression. For
+ * example, because the value is stored in a local variable.
+ */
+predicate outlivesFullExpr(Expr e) {


I want to point out that this has changed as well as been moved so the changes need reviewing as well

Now that I think about it I should have probably moved in a separate commit to the actual changes. I don't mind doing that if you want.

Oh, you're right. Thanks for pointing that out. I can see that there are some subtle changes here 🤔 Let me just think about the implication of this

I think the inclusion of not e.getConversion*().hasLValueToRValueConversion() in all the cases is a bad idea. For instance, consider this example that we currently catch on main:

const char* s = "hello"; const char* s13 = b1 ? std::string("hello").c_str() : s;

this is bad because of temporary std::string string is destroyed, but because there's an lvalue-to-rvalue conversion on the other branch of the ternary we won't flag this case any longer.

... We should actually add this test to the UseOfStringAfterLifetimeEnds testcases

I seems that the entire conditional is marked as having an hasLValueToRValueConversion as well as the branch s. But surely the conversion only needs to happen for s. The clang AST shows that. However our AST doesn't. Reading cppreference it suggest this is an rvalue so we don't need a a conversion here.

I can fix this by looking for this specific case but am I misunderstanding this case?

No, I think you're understanding this correctly. I think this is case 6.1 in https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator. I think we should just exclude the ternary case from the not e.getConversion*().hasLValueToRValueConversion() conjunct.

MathiasVP

Only a single suggestion, but otherwise this LGTM! Let's see if we can get a docs review in before your holidays start 🤞. Otherwise, I'll be happy to pick this up on Monday

MathiasVP · 2023-12-15T11:13:57Z

cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/UseOfStringAfterLifetimeEnds/test.cpp

@@ -190,6 +190,8 @@ const char* test1(bool b1, bool b2) {
  char* s9;
  s9 = std::string("hello").data(); // BAD

+  const char* s13 = b1 ? std::string("hello").c_str() : s1; // BAD


Thanks for adding this test 🙇

cpp/ql/src/Security/CWE/CWE-416/Temporaries.qll

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

github-actions bot added C++ documentation labels Dec 12, 2023

alexet force-pushed the unique-pointer-temporary branch from 24532cd to 53729c6 Compare December 12, 2023 16:21

CPP: Add query for detecting invalid uses of temporary unique pointers.

e9bc5a5

alexet force-pushed the unique-pointer-temporary branch from 53729c6 to e9bc5a5 Compare December 12, 2023 16:22

MathiasVP reviewed Dec 12, 2023

View reviewed changes

alexet marked this pull request as ready for review December 12, 2023 16:44

alexet requested a review from a team as a code owner December 12, 2023 16:44

alexet marked this pull request as draft December 12, 2023 16:45

MathiasVP reviewed Dec 12, 2023

View reviewed changes

CPP: Add change note for cpp/use-of-uniwue-pointer-after-lifetime-ends

136a77b

alexet marked this pull request as ready for review December 12, 2023 16:48

MathiasVP reviewed Dec 12, 2023

View reviewed changes

cpp/ql/src/Security/CWE/CWE-416/UseOfUniquePointerAfterLifetimeEnds.ql Outdated Show resolved Hide resolved

cpp/ql/src/change-notes/2023-12-12-use-of-unique-pointer-after-lifetime-ends.md Outdated Show resolved Hide resolved

CPP: Apply suggestions from code review

236a6a1

Fix spelling in query id Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

alexet commented Dec 12, 2023

View reviewed changes

CPP: Fix handling of ternary operators in tempory queries and add tests.

49e1467

MathiasVP reviewed Dec 15, 2023

View reviewed changes

MathiasVP added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Dec 15, 2023

alexet and others added 2 commits December 15, 2023 11:23

Update cpp/ql/src/Security/CWE/CWE-416/Temporaries.qll

c68d3c5

Co-authored-by: Mathias Vorreiter Pedersen <mathiasvp@github.com>

CPP: Format Temporaries

8e92fcc

CPP: Add query for detecting invalid uses of temporary unique pointers. #15078

CPP: Add query for detecting invalid uses of temporary unique pointers. #15078

alexet commented Dec 12, 2023 •

edited

github-actions bot commented Dec 12, 2023 •

edited

Use of unique pointer after lifetime ends

Recommendation

Example

References

MathiasVP left a comment

MathiasVP left a comment •

edited

alexet Dec 12, 2023

MathiasVP Dec 13, 2023 •

edited

MathiasVP Dec 13, 2023 •

edited

MathiasVP Dec 13, 2023

alexet Dec 13, 2023

MathiasVP Dec 13, 2023 •

edited

MathiasVP left a comment

MathiasVP Dec 15, 2023

CPP: Add query for detecting invalid uses of temporary unique pointers. #15078

Are you sure you want to change the base?

CPP: Add query for detecting invalid uses of temporary unique pointers. #15078

Conversation

alexet commented Dec 12, 2023 • edited

github-actions bot commented Dec 12, 2023 • edited

Use of unique pointer after lifetime ends

Recommendation

Example

References

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP left a comment • edited

Choose a reason for hiding this comment

alexet Dec 12, 2023

Choose a reason for hiding this comment

MathiasVP Dec 13, 2023 • edited

Choose a reason for hiding this comment

MathiasVP Dec 13, 2023 • edited

Choose a reason for hiding this comment

MathiasVP Dec 13, 2023

Choose a reason for hiding this comment

alexet Dec 13, 2023

Choose a reason for hiding this comment

MathiasVP Dec 13, 2023 • edited

Choose a reason for hiding this comment

MathiasVP left a comment

Choose a reason for hiding this comment

MathiasVP Dec 15, 2023

Choose a reason for hiding this comment

alexet commented Dec 12, 2023 •

edited

github-actions bot commented Dec 12, 2023 •

edited

MathiasVP left a comment •

edited

MathiasVP Dec 13, 2023 •

edited

MathiasVP Dec 13, 2023 •

edited

MathiasVP Dec 13, 2023 •

edited