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

calumgrant · 2024-10-15T14:50:08Z

Fixes false positives cause by buildless extraction. Commit-by-commit review recommended.

Removes 1008 false positives in the asterisk/asterisk projects as observed on codeql-qa.

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.

C++: Change note

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql

jketema · 2024-10-16T10:56:50Z

cpp/ql/lib/semmle/code/cpp/exprs/Call.qll

@@ -300,6 +300,11 @@ class FunctionCall extends Call, @funbindexpr {
    this.isVirtual() or
    this.getTarget().getAnAttribute().getName() = "weak"
  }
+
+  override predicate mayBeFromImplicitlyDeclaredFunction() {
+    this.getType() instanceof IntType and


How can this be anything else than int?

I don't think it can be, so perhaps this test is redundant. But, I'm wondering what happens if a function is implicit in some compilation units, but explicit in others?

It sound like that's something that need further thought, especially with regard to the implications that has for the changes made here?

I've added a test for this scenario, and removed the redundant test. So it seems that Function::getType() can return multiple things if there are conflicting declarations.

jketema

This looks somewhat odd, as the modifications differ across sprintf and fprintf, and as printf itself is left untouched.

jketema · 2024-10-16T13:29:17Z

cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll

@@ -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 }


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?

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.

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

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

jketema · 2024-10-16T13:29:46Z

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

@@ -534,6 +534,9 @@ class Expr extends StmtParent, @expr {

  /** Gets the function containing this control-flow node. */
  override Function getControlFlowScope() { result = this.getEnclosingFunction() }
+
+  /** Holds if this expression could be the return value of an implicitly declared function. */
+  predicate mayBeFromImplicitlyDeclaredFunction() { none() }


Why not just make this a predicate of Call?

Because we need to ask an Expr if it came from an implicitly-declared function. I suppose, we could have done arg.(Call).mayBeFromImplicitlyDeclaredFunction()?

Yeah, I think that would be better.

jketema · 2024-10-16T13:34:11Z

cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md

+---
+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.


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.

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

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.

jketema · 2024-10-17T10:55:10Z

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/tests.c

+    printf("%s", 1); // BAD
+    printf("%s", implicit_function()); // GOOD - we should ignore the type
+    sprintf(0, "%s", ""); // GOOD
+    fprintf(0, "%s", ""); // GOOD


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

calumgrant · 2024-10-18T08:52:08Z

Will re-run DCA after current changes.

jketema · 2024-10-18T09:31:08Z

cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll

@@ -125,6 +125,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
    // The formatting function either has a definition in the snapshot, or all
    // `DeclarationEntry`s agree on the number of parameters (otherwise we don't
    // really know the correct number)
+    result > 0 and // Avoid invalid declarations


It would make more sense to me to make this condition part of the charPred of this abstract class. Reasoning: there are no arguments besides vararg ones, then there is no explicit argument for the formatting string, which would be very odd for a formatting function.

Maybe something like:

abstract class FormattingFunction extends ArrayFunction, TaintFunction { int firstFormatArgumentIndex; int numberOfExplicitParameters; FormattingFunction() { forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() | numberOfExplicitParameters = fde.getNumberOfParameters() ) and ( if this.hasDefinition() then firstFormatArgumentIndex = this.getDefinition().getNumberOfParameters() else firstFormatArgumentIndex = numberOfExplicitParameters ) and firstFormatArgumentIndex > 0 }

and implement the relevant get predicates in terms of firstFormatArgumentIndex and numberOfExplicitParameters.

I think it would also make sense at this point to move getAnExplicitDeclarationEntry to Function.

Will this also work for intrinsic functions like __builtin___sprintf_chk? I will try.

Intrinsic functions should still have a prototype in the database.

I tried this out, and encountered a couple of problems:

Certain dataflow functions no longer identify the formatting functions as sinks if the functions have not been declared. Maybe this is ok, but I needed to fix up some of the qltests to add missing function declarations.

The builtin functions have no declaration entries - I tested this using count(fn.getADeclarationEntry()) which returned 0. This is fine, but I did need to add a special case to FormattingFunction.

I'll need to run DCA on this (once the tests pass), fix changenotes, and fix the tests on semmle-code.

Are you still ok with this design? @jketema

The builtin functions have no declaration entries - I tested this using count(fn.getADeclarationEntry()) which returned 0. This is fine, but I did need to add a special case to FormattingFunction.

That's an extractor bug, if it's reproducible in an integration test. Otherwise it's a bug in how we run tests.

I've looked at this a bit:

The question mark in #114=@"fun_decl___builtin___sprintf_chk(?)" is there, because the compiler generated function declaration is a C function, so name-mangling of the parameters should be ignored.

For compiler generated functions we apparently do not output a row in the fun_decls relation, which explains why you get a count of zero.

We do output parameter information directly on the function:
#13d=@"fun_decl___builtin___snprintf_chk(?)" .push * functions(#13d, "__builtin___snprintf_chk", 6) link_parent(#13d, #101) function_return_type(#13d, #10e) #13d_13e=@"{#13d}_0_par" params(#13d_13e, #13d, 0, #116) #13d_13f=@"{#13d}_1_par" params(#13d_13f, #13d, 1, #119) #13d_140=@"{#13d}_2_par" params(#13d_140, #13d, 2, #10e) #13d_141=@"{#13d}_3_par" params(#13d_141, #13d, 3, #119) #13d_142=@"{#13d}_4_par" params(#13d_142, #13d, 4, #11c) .pop

Questions (I'll also have a look at whether I can answer this myself):

Why do we look at the declaration entries and not at the functions?

Should we sometimes fallback to the parameter information of the Function if there are no declaration entries?

We look at declaration entries in order to figure out if the declaration was implicit or not. There can be spurious function parameters caused by extraction errors, so we need to double-check the function declaration entries.

I've opened a PR for an additional internal test related to this. I think the right solution here is to use the information from the Function and not the FunctionDeclarationEntry when we see that the function is a builtin function. In that case the information from the Function should be reliable, because the function's signature is under control of out frontend.

I originally added if this instanceof BuiltinFunction then firstFormatArgumentIndex = this.getNumberOfParameters(), because getNumberOfParameters() was reliable for builtins. I'm happy to add this back, because we shouldn't rely on FunctionDeclarationEntry for builtins. If it's ok with you, shall this be the final solution?

That sounds good.

jketema · 2024-10-18T09:42:20Z

cpp/ql/src/change-notes/2024-10-15-wrong-type-format-argument.md

+---
+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.


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.

jketema

LGTM. Could we run a final DCA experiment to see everything is ok?

github-actions bot added C++ documentation labels Oct 15, 2024

calumgrant force-pushed the calumgrant/bmn/wrong-type-format-arguments-test branch from 526aaff to 23970f1 Compare October 15, 2024 14:53

C++: Add test for cpp/wrong-type-format-argument

fe85e00

calumgrant force-pushed the calumgrant/bmn/wrong-type-format-arguments-test branch from 23970f1 to 1e459ac Compare October 15, 2024 15:09

C++: Clean up false-positives

853128c

C++: Change note

calumgrant force-pushed the calumgrant/bmn/wrong-type-format-arguments-test branch from 1e459ac to 853128c Compare October 16, 2024 08:46

calumgrant commented Oct 16, 2024

View reviewed changes

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql Outdated Show resolved Hide resolved

calumgrant added 2 commits October 16, 2024 10:23

C++: Implement Expr::mayBeFromImplicitlyDeclaredFunction

6a48ad0

C++: Change note for mayBeFromImplicitlyDeclaredFunction

d88a674

calumgrant marked this pull request as ready for review October 16, 2024 09:59

calumgrant requested a review from a team as a code owner October 16, 2024 09:59

jketema reviewed Oct 16, 2024

View reviewed changes

calumgrant added 2 commits October 16, 2024 13:46

C++: Add test for mixed implicit/explicit function declarations

ceceee1

C++: Remove redundant test

9758e02

jketema reviewed Oct 16, 2024

View reviewed changes

calumgrant added 2 commits October 16, 2024 15:47

C++: Tweak test

5315a5c

C++: Reject invalid results from getFirstFormatArgumentIndex()

4341fab

jketema reviewed Oct 17, 2024

View reviewed changes

jketema reviewed Oct 18, 2024

View reviewed changes

calumgrant added 3 commits October 18, 2024 14:52

C++: Resolve firstFormatArgumentIndex in FormattingFunction CP

4197805

C++: Move mayBeFromImplicitlyDeclaredFunction to Call

0fcabc4

C++: Fix CWE-022

c5a082f

calumgrant force-pushed the calumgrant/bmn/wrong-type-format-arguments-test branch from 6463531 to c5a082f Compare October 21, 2024 12:36

calumgrant added 2 commits October 23, 2024 14:35

C++: Handle builtin FormattingFunctions better

f37be68

C++: Update change notes

421413a

jketema approved these changes Oct 23, 2024

View reviewed changes

calumgrant merged commit a8f1d57 into main Oct 24, 2024
16 checks passed

calumgrant deleted the calumgrant/bmn/wrong-type-format-arguments-test branch October 24, 2024 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

calumgrant commented Oct 15, 2024 •

edited

Loading

jketema Oct 16, 2024

calumgrant Oct 16, 2024

jketema Oct 16, 2024

calumgrant Oct 16, 2024

jketema left a comment

jketema Oct 16, 2024

calumgrant Oct 16, 2024

calumgrant Oct 16, 2024

calumgrant Oct 17, 2024

jketema Oct 16, 2024

calumgrant Oct 16, 2024

jketema Oct 17, 2024

jketema Oct 16, 2024

calumgrant Oct 17, 2024

jketema Oct 18, 2024

jketema Oct 17, 2024

calumgrant Oct 17, 2024

calumgrant commented Oct 18, 2024

jketema Oct 18, 2024

calumgrant Oct 18, 2024

jketema Oct 18, 2024

calumgrant Oct 21, 2024

jketema Oct 21, 2024 •

edited

Loading

jketema Oct 22, 2024 •

edited

Loading

calumgrant Oct 22, 2024

jketema Oct 23, 2024

calumgrant Oct 23, 2024

jketema Oct 23, 2024

jketema Oct 18, 2024

jketema left a comment

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

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

Conversation

calumgrant commented Oct 15, 2024 • edited Loading

Pull Request checklist

All query authors

Internal query authors only

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calumgrant commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

jketema Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jketema left a comment

Choose a reason for hiding this comment

calumgrant commented Oct 15, 2024 •

edited

Loading

jketema Oct 21, 2024 •

edited

Loading

jketema Oct 22, 2024 •

edited

Loading