C++: Remove FPs from cpp/uninitialized-local when encountered extraction errors

[calumgrant]

Fixes false-positives in standalone extraction mode, whereby local variables may be initialized using function calls to unrecognised functions. These manifest as ErrorExpr, so we can exclude these from the results.

The DCA results are interesting. We lose one FP on abseil (this is good), but also a bunch of results on the samate project, which are due to an ErrorExpr creeping into one of the C functions, which are all called goodB2G or bad, so seem to identify as the same function. However, the root cause of the ErrorExpr is unclear as there's nothing obvious in the extractor logs.

Questions:

1. Are we ok with the DCA results? Should we merge this as-is?
2. Change-note required? (Will fail PR-check)

Pull Request checklist

All query authors

• A change note is added if necessary. See the documentation in this repository.
• All new queries have appropriate .qhelp. See the documentation in this repository.
• QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
• New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

• Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
• Changes are validated at scale (internal access required).
• Adding a new query? Consider also adding the query to autofix.

[jketema]

The tests for this query are located in cpp/test/query-tests/Security/CWE/CWE-457/semmle/tests. We should probably add the test case there.

[calumgrant]

Fine by me.

[jketema]

The DCA results are interesting. We lose one FP on abseil (this is good), but also a bunch of results on the samate project, which are due to an ErrorExpr creeping into one of the C functions, which are all called goodB2G or bad, so seem to identify as the same function. However, the root cause of the ErrorExpr is unclear as there's nothing obvious in the extractor logs.

This is for the same reason as:

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

Lines 151 to 157 in d0831eb

	// The extractor deliberately emits an `ErrorExpr` as the first argument to
	// the allocator call, if any, of a `NewOrNewArrayExpr`. That `ErrorExpr`
	// should not be translated.
	exists(NewOrNewArrayExpr new | expr = new.getAllocatorCall().getArgument(0))
	or
	not translateFunction(getEnclosingFunction(expr)) and
	not Raw::varHasIRFunc(getEnclosingVariable(expr))

So we need to be a bit more careful here. We should probably define a predicate that captures this case and also use it in other queries that check for ErrorExpr.

[calumgrant]

The DCA results are interesting. We lose one FP on abseil (this is good), but also a bunch of results on the samate project, which are due to an ErrorExpr creeping into one of the C functions, which are all called goodB2G or bad, so seem to identify as the same function. However, the root cause of the ErrorExpr is unclear as there's nothing obvious in the extractor logs.

This is for the same reason as:

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

Lines 151 to 157 in d0831eb

	// The extractor deliberately emits an `ErrorExpr` as the first argument to
	// the allocator call, if any, of a `NewOrNewArrayExpr`. That `ErrorExpr`
	// should not be translated.
	exists(NewOrNewArrayExpr new | expr = new.getAllocatorCall().getArgument(0))
	or
	not translateFunction(getEnclosingFunction(expr)) and
	not Raw::varHasIRFunc(getEnclosingVariable(expr))

So we need to be a bit more careful here. We should probably define a predicate that captures this case and also use it in other queries that check for ErrorExpr.

That certainly matches what I've observed. Thanks for the pointer.

[jketema]

Note that there's something similar going on with the first argument of a deallocator call of a delete expressions. And I'm now left wondering whether we handle that correctly in the IR (will briefly investigate).

[calumgrant]

Note that there's something similar going on with the first argument of a deallocator call of a delete expressions. And I'm now left wondering whether we handle that correctly in the IR (will briefly investigate).

This does leave me wondering why this expression is emitted at all. I can work around it for now, but a follow-up could be to replace this with something else or avoid it entirely.

I've not looked into the IR, but it does seem to me that ignoreExprOnly(Expr expr) should also consider DeleteOrDeleteArrayExpr.

[jketema]

I've not looked into the IR, but it does seem to me that ignoreExprOnly(Expr expr) should also consider DeleteOrDeleteArrayExpr.

That only makes sense if the IR actually looks at that argument during IR generation, which I'm not sure of.

This does leave me wondering why this expression is emitted at all. I can work around it for now, but a follow-up could be to replace this with something else or avoid it entirely.

The expression is an argument to a call, so something needs to be there. Looking briefly at the extractor code it didn't seem completely straightforward to replace this with what it really should be.

[jketema]

I've not looked into the IR, but it does seem to me that ignoreExprOnly(Expr expr) should also consider DeleteOrDeleteArrayExpr.

I was mistaken. We only output the error expression in the case of new, and not delete. The extractor code is pretty difficult to follow here.

[jketema]

I'm not a particular fan of having this predicate, as it exposes something that shouldn't really be exposed.

[calumgrant]

I wasn't sure about this name. You suggested I should encapsulate the functionality from TranslatedElement.qll, but I'm not sure how helpful this really is. Could also rename it to isExpected(), but it's still just a workaround really. Happy to remove this predicate and inline it as it's not really adding much value.

[jketema]

Sorry for the misunderstanding, I did not suggest encapsulating the functionality from TranslatedElement.qll. I only tried to suggest to do something similar. I don't think we need this predicate for that. It's something that can just be inlined in the hasErrors predicate you introduced.

[calumgrant]

I think that would be cleaner.

[calumgrant]

I've not looked into the IR, but it does seem to me that ignoreExprOnly(Expr expr) should also consider DeleteOrDeleteArrayExpr.

I was mistaken. We only output the error expression in the case of new, and not delete. The extractor code is pretty difficult to follow here.

I couldn't create a test case that generated the deallocator arg, so I decided to ignore it.

[jketema]

I couldn't create a test case that generated the deallocator arg, so I decided to ignore it.

Yup, because: "We only output the error expression in the case of new, and not delete."

[MathiasVP]

Might as well stick this constraint into isSinkImpl to speed up the recursion

[jketema]

I think the comment should be retained here.

[jketema]

Or at least something along those lines.

[jketema]

LGTM. Just want to see some updated DCA results.

[calumgrant]

Will add change note.