Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Nov 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4d85144
C++: Remove FPs from cpp/too-few-arguments
calumgrant Nov 6, 2024
227f9c7
C++: Update the test.c comments
calumgrant Nov 22, 2024
4fa8c6a
C++: Add change note
calumgrant Nov 22, 2024
60155ce
C++: Address review comments
calumgrant Nov 22, 2024
3b4fdb3
Update cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/t…
calumgrant Nov 22, 2024
4dab039
C++: Update expected output
calumgrant Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,7 @@ predicate tooFewArguments(FunctionCall fc, Function f) {
hasDefiniteNumberOfParameters(fde)
|
fde.getNumberOfParameters() > fc.getNumberOfArguments()
)
) and
// Don't report on implicit function declarations, as these are likely extraction errors.
not f.getADeclarationEntry().isImplicit()
Comment on lines +55 to +56
Copy link
Contributor

@jketema jketema Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@geoffw0 geoffw0 Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

}
4 changes: 4 additions & 0 deletions cpp/ql/src/change-notes/2024-11-22-too-few-arguments.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* 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.
2 changes: 0 additions & 2 deletions cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/TooFewArguments.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
| test.c:34:3:34:19 | call to not_yet_declared2 | This call has fewer arguments than required by $@. | test.c:32:3:32:3 | not_yet_declared2 | not_yet_declared2 |
| test.c:34:3:34:19 | call to not_yet_declared2 | This call has fewer arguments than required by $@. | test.c:76:6:76:22 | not_yet_declared2 | not_yet_declared2 |
Comment on lines -1 to -2
Copy link
Contributor

@jketema jketema Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| test.c:36:3:36:29 | call to declared_empty_defined_with | This call has fewer arguments than required by $@. | test.c:77:6:77:32 | declared_empty_defined_with | declared_empty_defined_with |
| test.c:87:10:87:20 | call to dereference | This call has fewer arguments than required by $@. | test.c:90:5:90:15 | dereference | dereference |
4 changes: 2 additions & 2 deletions cpp/ql/test/query-tests/Likely Bugs/Underspecified Functions/test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ void test(int *argv[]) {

not_yet_declared1(1); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
not_yet_declared2(1); // BAD (GOOD for everything except for cpp/implicit-function-declaration)
not_yet_declared2(ca); // BAD
not_yet_declared2(); // BAD
not_yet_declared2(ca); // BAD (GOOD for everything except for cpp/mistyped-function-arguments)
calumgrant marked this conversation as resolved.
Show resolved Hide resolved
not_yet_declared2(); // BAD [NOT DETECTED] (GOOD for everything except for cpp/too-few-arguments)

declared_empty_defined_with(); // BAD
declared_empty_defined_with(1); // GOOD
Expand Down