Skip to content

C++: Add some test cases for cpp/wrong-type-format-argument #21421

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

Open
geoffw0 wants to merge 3 commits into
base: main
Choose a base branch
from
+41 −0
Open

C++: Add some test cases for cpp/wrong-type-format-argument #21421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d23a3f8
C++: Add a test case for WrongTypeFormatArguments involving code that…
geoffw0 Mar 5, 2026
7f6fd34
C++: Expose a type resolution issue.
geoffw0 Mar 6, 2026
da99d36
C++: Turns out we can simplify.
geoffw0 Mar 6, 2026
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
8 changes: 8 additions & 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
@@ -1 +1,9 @@
| second.cpp:13:19:13:19 | s | This format specifier for type 'size_t' does not match the argument type '..(*)(..)'. |
| second.cpp:14:19:14:19 | s | This format specifier for type 'size_t' does not match the argument type '..(*)(..)'. |
| second.cpp:15:18:15:18 | s | This format specifier for type 'int' does not match the argument type '..(*)(..)'. |
| second.cpp:16:19:16:19 | s | This format specifier for type 'long' does not match the argument type '..(*)(..)'. |
| second.cpp:17:20:17:20 | s | This format specifier for type 'long long' does not match the argument type '..(*)(..)'. |
| second.cpp:18:18:18:18 | s | This format specifier for type 'unsigned int' does not match the argument type '..(*)(..)'. |
| second.cpp:26:18:26:39 | ... - ... | This format specifier for type 'int' does not match the argument type 'long'. |
| second.cpp:29:18:29:39 | ... - ... | This format specifier for type 'unsigned int' does not match the argument type 'long'. |
| tests.c:7:18:7:18 | 1 | This format specifier for type 'char *' does not match the argument type 'int'. |
3 changes: 3 additions & 0 deletions cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/first.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

// defines type size_t plausibly
typedef unsigned long size_t;
Comment on lines +2 to +3
Copy link
Contributor

@jketema jketema Mar 9, 2026

Choose a reason for hiding this comment

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

Do we need this file at all?

Copy link
Contributor Author

@geoffw0 geoffw0 Mar 9, 2026

Choose a reason for hiding this comment

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

Without this line we lose two of the results, and they're the two closest to the real world cases I was looking at:

-| second.cpp:13:19:13:19 | s | This format specifier for type 'size_t' does not match the argument type '..(*)(..)'. |
-| second.cpp:14:19:14:19 | s | This format specifier for type 'size_t' does not match the argument type '..(*)(..)'. |

Though we would retain the other results involving function pointer types, for example:

 | second.cpp:15:18:15:18 | s | This format specifier for type 'int' does not match the argument type '..(*)(..)'. |

What we can't do is move the definition of size_t into second.cpp. That makes the entire phenomena disappear.

TL;DR: I'd prefer to keep this as it better models real world cases; but it's not strictly necessary to reproduce an issue.

30 changes: 30 additions & 0 deletions cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Buildless/second.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// semmle-extractor-options: --expect_errors

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

// defines type `myFunctionPointerType`, referencing `size_t`
typedef size_t (*myFunctionPointerType) ();

void test_size_t() {
size_t s = 0;
Copy link
Contributor

@jketema jketema Mar 9, 2026

Choose a reason for hiding this comment

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

Maybe we should add a comment on what our frontend thinks size_t is at this point?

Copy link
Contributor Author

@geoffw0 geoffw0 Mar 9, 2026

Choose a reason for hiding this comment

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

There are two size_t types in the database, both are typedefs but only one is a typedef to an integral type as we would expect. I think this fits the theory that the definition of myFunctionPointerType is somehow creating an alternative size_t typedef type.

But which type does the variable "s" have? ... it looks like there is no Variable "s" extracted in the database. I only see variables "buffer" and "format" in this file. Thus, there are also no VariableAccesses to "s" in the printf calls, I see FunctionAccesses instead, understandably with function types. This seems to be the direct cause of the spurious results I'm seeing.

If I take the definition of myFunctionPointerType away, the variable "s" then exists with an error type, and it looks like we then don't extract the access to "s" at all (which means we don't get erroneous query results).

So I think we have two paths available to fixing this:

  1. fix the extraction of myFunctionPointerType so that it doesn't spuriously generate a size_t typedef, perhaps by simply erroring on this line. This approach has the upside of potentially resolving other related issues.
  2. patch the query to ignore arguments that are accesses to functions with an erroneous type (or just: that are function accesses). This approach has the advantage of being simple and immediate.


printf("%zd", s); // GOOD
printf("%zi", s); // GOOD
printf("%zu", s); // GOOD [FALSE POSITIVE]
printf("%zx", s); // GOOD [FALSE POSITIVE]
printf("%d", s); // BAD
printf("%ld", s); // BAD
printf("%lld", s); // BAD
printf("%u", s); // BAD

char buffer[1024];

printf("%zd", &buffer[1023] - buffer); // GOOD
printf("%zi", &buffer[1023] - buffer); // GOOD
printf("%zu", &buffer[1023] - buffer); // GOOD
printf("%zx", &buffer[1023] - buffer); // GOOD
printf("%d", &buffer[1023] - buffer); // BAD
printf("%ld", &buffer[1023] - buffer); // BAD [NOT DETECTED]
printf("%lld", &buffer[1023] - buffer); // BAD [NOT DETECTED]
Comment on lines +27 to +28
Copy link
Contributor

@jketema jketema Mar 9, 2026

Choose a reason for hiding this comment

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

Why are these bad?

Copy link
Contributor Author

@geoffw0 geoffw0 Mar 9, 2026

Choose a reason for hiding this comment

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

Because the pointer type is ssize_t not long int, and hose types may or may-not have the same size depending on platform. Though ... in the interests of filtering out results considered noisy, we've been increasingly lenient on this kind of thing in this query. The user may know more about the platform than we do.

Copy link
Contributor

@jketema jketema Mar 9, 2026

Choose a reason for hiding this comment

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

Because the pointer type is ssize_t not long int, and hose types may or may-not have the same size depending on platform.

Ok. I think I misunderstand why we're running into problems with the query. There seem to be at least three reasons going by the tests you're adding here: size_t not defined, size_t defined elsewhere, some other reason related to pointers that I don't yet understand. Is that correct?

Copy link
Contributor Author

@geoffw0 geoffw0 Mar 9, 2026

Choose a reason for hiding this comment

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

The primary issue is the results where the query claims the argument has a function type '..(*)(..)', when it doesn't. These messages are incorrect and confusing.

printf("%u", &buffer[1023] - buffer); // BAD
}