C++: Add an experimental query for surprising lifetimes from range-based for loops

[MathiasVP]

This PR adds an experimental query to detect issues such as:

std::vector<std::vector<int>> CreateCollection();

void use(...);

void test() {
  // BUG: CreateCollection() is destroyed before CreateCollection()[0].begin() is called
  for (auto x : CreateCollection()[0]) {
    use(x);
  }
}

I think the precision of this query is actually perfectly suited for Code Scanning (🤞), but since the query depends on ongoing work on both the extractor side and the analysis side it'll start its lifetime (pun very much intended!) in experimental.

All the NOT DETECTED tests are actually detected when running with the yet-to-be-released version of the extractor and with #15858 merged.

I don't think it makes much sense to run this on DCA yet as that DCA run will mostly test #15858 (and the internal extractor changes) I guess. I'll try doing it anyway just for fun, though!

[github-actions]

QHelp previews:

cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.qhelp

Iterator to expired container

Using an iterator owned by a container after the lifetime of the container has expired can lead to undefined behavior. This is because the iterator may be invalidated when the container is destroyed, and dereferencing an invalidated iterator is undefined behavior. These problems can be hard to spot due to C++'s complex rules for temporary object lifetimes and their extensions.

Recommendation

Never create an iterator to a temporary container when the iterator is expected to be used after the container's lifetime has expired.

Example

The rules for lifetime extension ensures that the code in lifetime_of_temp_extended is well-defined. This is because the lifetime of the temporary container returned by get_vector is extended to the end of the loop. However, prior to C++23, the lifetime extension rules do not ensure that the container returned by get_vector is extended in lifetime_of_temp_not_extended. This is because the temporary container is not bound to a rvalue reference.

#include <vector>

std::vector<int> get_vector();

void use(int);

void lifetime_of_temp_extended() {
  for(auto x : get_vector()) {
    use(x); // GOOD: The lifetime of the vector returned by `get_vector()` is extended until the end of the loop.
  }
}

// Writes the the values of `v` to an external log and returns it unchanged.
const std::vector<int>& log_and_return_argument(const std::vector<int>& v);

void lifetime_of_temp_not_extended() {
  for(auto x : log_and_return_argument(get_vector())) {
    use(x); // BAD: The lifetime of the vector returned by `get_vector()` is not extended, and the behavior is undefined.
  }
}

References

• CERT C Coding Standard: MEM30-C. Do not access freed memory.
• OWASP: Using freed memory.
• Lifetime safety: Preventing common dangling
• Containers library
• Range-based for loop (since C++11)
• Common Weakness Enumeration: CWE-416.
• Common Weakness Enumeration: CWE-664.

[geoffw0]

This looks like a great query, and the .qhelp is a good explanation of the problem (with references for more detail). 👍

Some questions below.

We also need to do a DCA run, docs review, and a closer examination of real world results ... but for each of these, I don't really mind whether we do it now or when we're ready to promote the query out of experimental. It would certainly help to have the extractor change merged.

[MathiasVP]

We also need to do a DCA run, docs review, and a closer examination of real world results ... but for each of these, I don't really mind whether we do it now or when we're ready to promote the query out of experimental. It would certainly help to have the extractor change merged.

Partial answer: See what I wrote above:

I don't think it makes much sense to run this on DCA yet as that DCA run will mostly test #15858 (and the internal extractor changes) I guess. I'll try doing it anyway just for fun, though!

Basically, there will be no real world results before the extractor (and analysis) changes are merged. Even then, we have to wait for MRVA databases to update before we can even run this on MRVA.

[subatoi]

👍 from Docs

[MathiasVP]

I believe I've addressed all your comments now @geoffw0. In terms of

We also need to do a DCA run, docs review, and a closer examination of real world results ... but for each of these, I don't really mind whether we do it now or when we're ready to promote the query out of experimental. It would certainly help to have the extractor change merged.

I believe it's best merge this query, and then we can do proper MRVA result investigations and DCA tests once we move the query out of experimental (since, at that point, we'll have both the QL and extractor changes merged, and possibly even MRVA data depending on how lucky we are with the timing of a release 🤞)

[geoffw0]

and 👍 from me.