CPP: Fix WrongTypeFormatArguments.ql char16_t * issues (and others) #286
Conversation
Certain Microsoft projects, such as CoreCLR and ChakraCore, use a library called the PAL, which enables two-byte strings in the printf family of functions, even when built on a platform with four-byte strings. This adds support for determining the size of a wide character from the definitions of such functions, rather than assuming that they match the compiler's wchar_t.
…s most of the tests).
|
@geoffw0 there are conflicts even though this is a freshly opened PR. Based on the affected file names, I'm guessing that you edited them before all files in the repo got normalised to Unix line endings. Try to rebase on the latest master with |
|
I've fixed the conflicts, it was on an old base revision with two genuine conflicts (one in a test, one in a change note). Didn't notice any issue with line endings but let me know if there is one! |
| @@ -25,7 +25,8 @@ private predicate formattingFunctionCallExpectedType(FormattingFunctionCall ffc, | |||
| ffc.getTarget() = f and | |||
| f.getFormatParameterIndex() = i and | |||
| ffc.getArgument(i) = fl and | |||
| fl.getConversionType(pos) = expected | |||
| fl.getConversionType(pos) = expected and | |||
| count(fl.getConversionType(pos)) = 1 | |||
There was a problem hiding this comment.
I suspect this is only needed because the where clause in this query has its logic the wrong way around for when there are multiple expected types. I'd like to try fixing this, but I won't let it block this PR.
There was a problem hiding this comment.
I will look at this in a follow-up.
There was a problem hiding this comment.
To clarify, I'm thinking that
not trivialConversion(expected.getUnspecifiedType(), actual.getUnspecifiedType())
should be replaced by
not exists(Type anyExpected |
formatArgType(ffc, n, anyExpected, arg, actual) and
trivialConversion(anyExpected.getUnspecifiedType(), actual.getUnspecifiedType())
)
If it's a UI issue to have multiple alert messages for the same line, we could use strictconcat to concatenate the names of the expected types instead of having a Type expected in the from clause.
There was a problem hiding this comment.
You were right - see #1255 and https://git.semmle.com/Semmle/code/pull/31630.
| @@ -7,6 +7,38 @@ | |||
| */ | |||
|
|
|||
| import semmle.code.cpp.Function | |||
|
|
|||
| private Type stripTopLevelSpecifiersOnly(Type t) { | |||
There was a problem hiding this comment.
Why was it necessary to introduce this? The commit that introduced it didn't come with any test output changes.
There was a problem hiding this comment.
It does have test changes, but in a test that's still internal (see internal PR https://git.semmle.com/Semmle/code/pull/28289).
Having said this, looking at it again I think I can do better. I'll do this as follow-up work.
Fix type access expression extraction for function/property references
Fix handling of wide string types in 'WrongTypeFormatArguments.ql'. Previously they were assumed to always be
wchar_t *, which lead to high numbers of false positive results on projects where, say,char16_t *strings are used. Also fixed:%p.The first three commits are from an old pull request by @rdmarsh2, that was never merged due to concerns about mixing string types (which have been fixed here).
I have some smaller fixes to follow, but this PR should eliminate most of the false positives we've been looking at.
@jbj