C++: Improve documentation for cpp/iterator-to-expired-container

[MathiasVP]

This PR does two things that should hopefully make the results from the new cpp/iterator-to-expired-container query easier to understand:

• It updates the alert message to more correctly reflect what's going on
• It updates the qhelp file so that it contains an example of a fix.

Note that, because of the first fix, we only get half the number of alerts since we no longer include the call to begin in the alert message. Previously, we'd often get an alert on the same location with two alert messages such as:

• This object is destroyed before call to begin is called.
• This object is destroyed before call to end is called.

Other than the alert being wrong, it was also quite annoying to have two alerts on the same element explaining the same problem.

[github-actions]

QHelp previews:

cpp/ql/src/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.
  }
}

To fix lifetime_of_temp_not_extended, consider rewriting the code so that the lifetime of the temporary object is extended. In fixed_lifetime_of_temp_not_extended, the lifetime of the temporary object has been extended by storing it in an rvalue reference.

void fixed_lifetime_of_temp_not_extended() {
  auto&& v = get_vector();
  for(auto x : log_and_return_argument(v)) {
    use(x); // GOOD: The lifetime of the container returned by `get_vector()` has been extended to the lifetime of `v`.
  }
}

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]

Changes LGTM.

Is it worth getting a docs review for this small change? Come to think of it, did the .qhelp for this new query get a full review at some point?

[MathiasVP]

Is it worth getting a docs review for this small change?

Hm, yeah. That may be a good idea. I'll add a ready-for-docs-review label to this PR

Come to think of it, did the .qhelp for this new query get a full review at some point?

It did get a review when we merged it into experimental, yes: #15939

[mchammer01]

LGTM ✨
Two very minor suggestions from a Docs perspective.

[MathiasVP]

Thanks @mchammer01 🙇 All comments addressed!

[mchammer01]

LGTM