C++: Remove FPs from cpp/too-few-arguments

[calumgrant]

Fixes https://github.com/github/codeql-c-team/issues/2462

False-positives in this query were due to implicit C function definitions. Arguably, we shouldn't report this alert for implicit C function definitions, because these are much more likely to be extraction errors rather than actual problems.

The current tests pretty well covered the existing behaviour, so I just reused this test.

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]

I wonder if this is too strict, given the test results that disappear. Shouldn't this be something like:

• if all declaration entries are implicit, then ignore
• if the explicit declaration entries differ on the number of parameters, then ignore

Edit: fixed typos where I wrote "explicit", but meant "implicit", and vice versa.

[calumgrant]

One of the scenarios I'd like to handle is something like

file1.c:

// Decl/definition
void assert(pchar __file, unsigned int __line, pchar __function);

file2.c:

assert(x);  // Implicit fn decl because the assert macro isn't defined

In this case, some fndecl entries are explicit, and all explicit fndecls have the same number of parameters.

So I can write

forex(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
    fde.getNumberOfParameters() = f.getNumberOfParameters()
  )

but this basically covers the case above. If we weaken it to

  forex(FunctionDeclarationEntry fde | fde = f.getAnExplicitDeclarationEntry() |
    fde.getNumberOfParameters() = f.getNumberOfParameters()
  ) and

then this doesn't handle the assert case above and doesn't get rid of all the noise.

[geoffw0]

What if you move not f.getADeclarationEntry().isImplicit() into hasDefiniteNumberOfParameters?

[jketema]

If this is just to handle assert: I would expect there to be some macro definition of assert to be in scope. Unless the particular project you're looking at hand-rolled the definition, and we're not managing to pick up on the correct header file?

[calumgrant]

What if you move not f.getADeclarationEntry().isImplicit() into hasDefiniteNumberOfParameters?

That didn't remove the FPs. I think it's because implicit FunctionDeclarationEntry always have 0 parameters anyway.

[calumgrant]

If this is just to handle assert: I would expect there to be some macro definition of assert to be in scope. Unless the particular project you're looking at hand-rolled the definition, and we're not managing to pick up on the correct header file?

The FPs have also appeared on projects that don't involve macros or assert. In the case of assert, most likely the wrong header file was included or couldn't be found.

It would be possible to remove some FPs simply by saying and not exists(Macro m | m.getName() = f.getName()) but that's very specific and doesn't cover the cases where there isn't a macro involved.

In the general case, we'd hope to improve header file resolution, but we should expect this to go wrong at times.

[jketema]

I think it's because implicit FunctionDeclarationEntry always have 0 parameters anyway.

Correct. An implicit function declarations foo always have the form extern int foo().

[jketema]

Looking at a comparison between traced and BMN in DCA, I see that there are 0 results in the traced run and many (211) results in the BMN run. I wonder if the discussion here should actually be whether we should disable the query with BMN?

[calumgrant]

Looking at a comparison between traced and BMN in DCA, I see that there are 0 results in the traced run and many (211) results in the BMN run. I wonder if the discussion here should actually be whether we should disable the query with BMN?

I assume those 211 results are even with this fix?

[jketema]

I assume those 211 results are even with this fix?

That's before this fix.

[jketema]

@calumgrant Could you re-run the traced DCA experiment? Two projects seemed to have failed.

[jketema]

The test.c file probably needs to be updated to indicated that we do not detect these.

[jketema]

I think changing this to "GOOD" is incorrect. It's still "BAD", but we no longer detect it. For that we have "BAD [NOT DETECTED]". That probably does need some qualification here, as that only applies to cpp/too-few-arguments.

[jketema]

Suggested change

	* The "Call to function with fewer arguments than declared parameters" query (`cpp/too-few-arguments`) query produces no results if the function has been implicitly declared.
	* The "Call to function with fewer arguments than declared parameters" query (`cpp/too-few-arguments`) query no longer produces results if the function has been implicitly declared.

[jketema]

LGTM. My conclusion is there there probably not better way to do this, and given the kinds of results we saw in MRVA there also seems no need to come up with anything smarter.