C++: Add a new query for calling c_str on temporary objects

[MathiasVP]

This PR adds a new query that looks for variations of:

char* p = std::string("hello").c_str();
use(p);

MRVA results look really good. On the top 1000 projects I'm seeing 25 results, and they all look like TPs 🎉.

In the future I'd like to extend the query to support more lifetimes than just "temporary objects", but for now this is good enough, I think.

[github-actions]

QHelp previews:

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

Use of string after lifetime ends

Calling c_str on a std::string object returns a pointer to the underlying character array. However, if the std::string object is destroyed, then the pointer returned by c_str is no longer valid. If the pointer is used after the std::string object is destroyed, then the behavior is undefined.

Recommendation

Ensure that the pointer returned by c_str does not outlive the underlying std::string object.

Example

The following example concatenates two std::string objects, and then convert the resulting string to a C string using c_str so that it can be passed to the work function. However, the underlying std::string object that represents the concatenated string is destroyed as soon as the call to c_str returns. This means that work is given a pointer to invalid memory.

#include <string>
void work(const char*);

void work_with_combined_string_bad(std::string s1, std::string s2) {
  const char* combined_string = (s1 + s2).c_str();
  work(combined_string);
}

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

#include <string>
void work(const char*);

void work_with_combined_string_good(std::string s1, std::string s2) {
  auto combined_string = s1 + s2;
  work(combined_string.c_str());
}

References

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

[jketema]

Two small comments for now.

[geoffw0]

A couple of comments below.

Query, library changes, qhelp, test otherwise LGTM.

I'm looking at some real world results as well and will comment on those later.

[geoffw0]

Real world results: I did a smaller MRVA run and tested on some local databases as well. Found two results, both LGTM. 👍

[geoffw0]

I'm happy with this PR and I think it's a great new query. Looks like @jketema is happy too. DCA LGTM. I think we just need the docs team to take a look (I've taken the liberty of adding the ready-for-doc-review label).

[jketema]

I haven't looked at all the details, but I trust @geoffw0's judgement here.

[MathiasVP]

(I've taken the liberty of adding the ready-for-doc-review label).

Thank you. I was indeed going to do that as soon as I had gotten a 👍 from the team.

[felicitymay]

Thanks for the review request. This looks good. Just a couple of small comments.

[felicitymay]

We normally include a comment here. Something like this, but I'm sure you can write this more clearly:

Suggested change

	// BAD: `c_str` is destroyed on return so `work` acts on a pointer to invalid memory

[felicitymay]

Similarly, we should add a GOOD comment here.