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++: Reduce FPs in cpp/wrong-type-format-argument due to extraction errors #17775

Merged
merged 13 commits into from
Oct 24, 2024
Merged

C++: Reduce FPs in cpp/wrong-type-format-argument due to extraction errors #17775

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fe85e00
C++: Add test for cpp/wrong-type-format-argument
calumgrant Oct 15, 2024
853128c
C++: Clean up false-positives
calumgrant Oct 14, 2024
6a48ad0
C++: Implement Expr::mayBeFromImplicitlyDeclaredFunction
calumgrant Oct 16, 2024
d88a674
C++: Change note for mayBeFromImplicitlyDeclaredFunction
calumgrant Oct 16, 2024
ceceee1
C++: Add test for mixed implicit/explicit function declarations
calumgrant Oct 16, 2024
9758e02
C++: Remove redundant test
calumgrant Oct 16, 2024
5315a5c
C++: Tweak test
calumgrant Oct 16, 2024
4341fab
C++: Reject invalid results from getFirstFormatArgumentIndex()
calumgrant Oct 17, 2024
4197805
C++: Resolve firstFormatArgumentIndex in FormattingFunction CP
calumgrant Oct 18, 2024
0fcabc4
C++: Move mayBeFromImplicitlyDeclaredFunction to Call
calumgrant Oct 18, 2024
c5a082f
C++: Fix CWE-022
calumgrant Oct 18, 2024
f37be68
C++: Handle builtin FormattingFunctions better
calumgrant Oct 23, 2024
421413a
C++: Update change notes
calumgrant Oct 23, 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/lib/semmle/code/cpp/models/implementations/Printf.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ private class Fprintf extends FormattingFunction, NonThrowingFunction {
override int getFormatParameterIndex() { result = 1 }

override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }

override int getFirstFormatArgumentIndex() { result = 2 }
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks odd. It feels like there might be a bug in one of the super classes. Otherwise, why doesn't Printf have a similar predicate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't fully understand this, but it's what's needed to remove false-positives on the asterisk project. The declaration of int fprintf() in the last commit (5315a5c) replicates what happened in the database. I briefly forgot why I needed that line.

This line is safe because class Fprintf always has 2 arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The root cause is actually that the extractor sometimes fails to mark function declarations as implicit. Hmm. Maybe that's the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed instead using 4341fab. I couldn't find a nice way to identify these extraction errors but hopefully the solution makes sense.

}

/**
Expand Down Expand Up @@ -91,7 +93,7 @@ private class Sprintf extends FormattingFunction, NonThrowingFunction {
override int getFirstFormatArgumentIndex() {
if this.hasName("__builtin___sprintf_chk")
then result = 4
else result = this.getNumberOfParameters()
else result = this.getNumberOfExplicitParameters()
}
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
* from implicit function declarations. If there is some inconsistency in the number
* of parameters, then don't return anything.
*/
private int getNumberOfExplicitParameters() {
int getNumberOfExplicitParameters() {
forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() |
result = fde.getNumberOfParameters()
)
Expand Down
3 changes: 2 additions & 1 deletion cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ where
) and
not arg.isAffectedByMacro() and
not arg.isFromUninstantiatedTemplate(_) and
not actual.getUnspecifiedType() instanceof ErroneousType
not actual.getUnspecifiedType() instanceof ErroneousType and
not arg.(Call).getTarget().getADeclarationEntry().isImplicit()
calumgrant marked this conversation as resolved.
Show resolved Hide resolved
select arg,
"This format specifier for type '" + expected.getName() + "' does not match the argument type '" +
actual.getUnspecifiedType().getName() + "'."
4 changes: 4 additions & 0 deletions cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Fixed false positives in the `cpp/wrong-type-format-argument` ("Wrong type of arguments to formatting function") query if there are extraction errors in the function.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically not true, as there are valid cases where a function is implicitly defined and correctly being used as a argument in a printf call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it might also reject true-positives. Do you have a suggested text I could use?

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 we mostly want to be explicit about what we ignore, i.e., calls to functions that are implicitly declared, as in the majority of cases we see that these are due to extraction errors, and not because the codebase is pre-C99 C-code, which allows implicit function declarations.

1 change: 1 addition & 0 deletions ...s/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| tests.c:6:18:6:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. |
1 change: 1 addition & 0 deletions ...ests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/WrongTypeFormatArguments.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql
10 changes: 10 additions & 0 deletions cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// semmle-extractor-options: --expect_errors

int printf(const char * format, ...);

void f() {
printf("%s", 1); // BAD
printf("%s", implicit_function()); // GOOD - we should ignore the type
sprintf(0, "%s", ""); // GOOD
fprintf(0, "%s", ""); // GOOD
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is now good, because int fprintf() now no longer yields a value for getFirstFormatArgumentIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

}
Loading