Merge pull request #286 from geoffw0/wrongtype16

CPP: Fix WrongTypeFormatArguments.ql char16_t * issues (and others)
This commit is contained in:
Jonas Jensen
2018-10-08 09:50:02 +02:00
committed by GitHub
26 changed files with 359 additions and 43 deletions

View File

@@ -13,8 +13,8 @@
| **Query** | **Expected impact** | **Change** |
|----------------------------|------------------------|------------------------------------------------------------------|
| Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. |
| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. |
| Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
| Wrong type of arguments to formatting function | Fewer false positive results | False positive results involving typedefs have been removed. Expected argument types are determined more accurately, especially for wide string and pointer types. Custom (non-standard) formatting functions are also identified more accurately. |
## Changes to QL libraries

View File

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

View File

@@ -220,6 +220,32 @@ class FormatLiteral extends Literal {
getUse().getTarget().(FormattingFunction).isWideCharDefault()
}
/**
* Gets the default character type expected for `%s` by this format literal. Typically
* `char` or `wchar_t`.
*/
Type getDefaultCharType() {
result = getUse().getTarget().(FormattingFunction).getDefaultCharType()
}
/**
* Gets the non-default character type expected for `%S` by this format literal. Typically
* `wchar_t` or `char`. On some snapshots there may be multiple results where we can't tell
* which is correct for a particular function.
*/
Type getNonDefaultCharType() {
result = getUse().getTarget().(FormattingFunction).getNonDefaultCharType()
}
/**
* Gets the wide character type for this format literal. This is usually `wchar_t`. On some
* snapshots there may be multiple results where we can't tell which is correct for a
* particular function.
*/
Type getWideCharType() {
result = getUse().getTarget().(FormattingFunction).getWideCharType()
}
/**
* Holds if this `FormatLiteral` is in a context that supports
* Microsoft rules and extensions.
@@ -629,7 +655,6 @@ class FormatLiteral extends Literal {
result = getConversionType2(n) or
result = getConversionType3(n) or
result = getConversionType4(n) or
result = getConversionType5(n) or
result = getConversionType6(n) or
result = getConversionType7(n) or
result = getConversionType8(n) or
@@ -696,33 +721,35 @@ class FormatLiteral extends Literal {
}
/**
* Gets the 'effective' string type character, that is, 's' (meaning a char string) or
* 'S' (meaning a wide string).
* - in the base case this is the same as the format type character.
* - for a `wprintf` or similar function call, the meanings are reversed.
* - the size prefixes 'l'/'w' (long) and 'h' (short) override the
* type character to effectively 'S' or 's' respectively.
* Gets the string type required by the nth conversion specifier.
* - in the base case this is the default for the formatting function
* (e.g. `char` for `printf`, `wchar_t` for `wprintf`).
* - the `%S` format character reverses wideness.
* - the size prefixes 'l'/'w' and 'h' override the type character
* to wide or single-byte characters respectively.
*/
private string getEffectiveStringConversionChar(int n) {
exists(string len, string conv | this.parseConvSpec(n, _, _, _, _, _, len, conv) and (conv = "s" or conv = "S") |
(len = "l" and result = "S") or
(len = "w" and result = "S") or
(len = "h" and result = "s") or
(len != "l" and len != "w" and len != "h" and (result = "s" or result = "S") and (if isWideCharDefault() then result != conv else result = conv))
)
}
private Type getConversionType4(int n) {
exists(string cnv, CharType t | cnv = this.getEffectiveStringConversionChar(n) |
cnv="s" and t = result.(PointerType).getBaseType()
and not t.isExplicitlySigned()
and not t.isExplicitlyUnsigned()
)
}
private Type getConversionType5(int n) {
exists(string cnv | cnv = this.getEffectiveStringConversionChar(n) |
cnv="S" and result.(PointerType).getBaseType().hasName("wchar_t")
exists(string len, string conv |
this.parseConvSpec(n, _, _, _, _, _, len, conv) and
(
(
(conv = "s" or conv = "S") and
len = "h" and
result.(PointerType).getBaseType() instanceof PlainCharType
) or (
(conv = "s" or conv = "S") and
(len = "l" or len = "w") and
result.(PointerType).getBaseType() = getWideCharType()
) or (
conv = "s" and
(len != "l" and len != "w" and len != "h") and
result.(PointerType).getBaseType() = getDefaultCharType()
) or (
conv = "S" and
(len != "l" and len != "w" and len != "h") and
result.(PointerType).getBaseType() = getNonDefaultCharType()
)
)
)
}

View File

@@ -12,7 +12,8 @@ class Printf extends FormattingFunction {
hasGlobalName("wprintf") or
hasGlobalName("wprintf_s") or
hasGlobalName("g_printf")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=0 }
@@ -26,7 +27,15 @@ class Printf extends FormattingFunction {
* The standard functions `fprintf`, `fwprintf` and their glib variants.
*/
class Fprintf extends FormattingFunction {
Fprintf() { this instanceof TopLevelFunction and (hasGlobalName("fprintf") or hasGlobalName("fwprintf") or hasGlobalName("g_fprintf"))}
Fprintf() {
this instanceof TopLevelFunction and
(
hasGlobalName("fprintf") or
hasGlobalName("fwprintf") or
hasGlobalName("g_fprintf")
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=1 }
override predicate isWideCharDefault() { hasGlobalName("fwprintf") }
@@ -47,7 +56,8 @@ class Sprintf extends FormattingFunction {
hasGlobalName("g_strdup_printf") or
hasGlobalName("g_sprintf") or
hasGlobalName("__builtin___sprintf_chk")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override predicate isWideCharDefault() {
@@ -100,7 +110,8 @@ class Snprintf extends FormattingFunction {
or hasGlobalName("g_snprintf")
or hasGlobalName("wnsprintf")
or hasGlobalName("__builtin___snprintf_chk")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() {
@@ -133,10 +144,13 @@ class Snprintf extends FormattingFunction {
* in the buffer.
*/
predicate returnsFullFormatLength() {
hasGlobalName("snprintf") or
hasGlobalName("g_snprintf") or
hasGlobalName("__builtin___snprintf_chk") or
hasGlobalName("snprintf_s")
(
hasGlobalName("snprintf") or
hasGlobalName("g_snprintf") or
hasGlobalName("__builtin___snprintf_chk") or
hasGlobalName("snprintf_s")
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getSizeParameterIndex() {
@@ -158,7 +172,8 @@ class StringCchPrintf extends FormattingFunction {
or hasGlobalName("StringCbPrintfEx")
or hasGlobalName("StringCbPrintf_l")
or hasGlobalName("StringCbPrintf_lEx")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() {
@@ -187,7 +202,8 @@ class Syslog extends FormattingFunction {
Syslog() {
this instanceof TopLevelFunction and (
hasGlobalName("syslog")
)
) and
not exists(getDefinition().getFile().getRelativePath())
}
override int getFormatParameterIndex() { result=1 }

View File

@@ -7,6 +7,38 @@
*/
import semmle.code.cpp.Function
private Type stripTopLevelSpecifiersOnly(Type t) {
(
result = stripTopLevelSpecifiersOnly(t.(SpecifiedType).getBaseType())
) or (
result = t and
not t instanceof SpecifiedType
)
}
/**
* A type that is used as a format string by any formatting function.
*/
Type getAFormatterWideType() {
exists(FormattingFunction ff |
result = stripTopLevelSpecifiersOnly(ff.getDefaultCharType()) and
result.getSize() != 1
)
}
/**
* A type that is used as a format string by any formatting function, or `wchar_t` if
* there is none.
*/
private Type getAFormatterWideTypeOrDefault() {
result = getAFormatterWideType() or
(
not exists(getAFormatterWideType()) and
result instanceof Wchar_t
)
}
/**
* A standard library function that uses a `printf`-like formatting string.
*/
@@ -20,6 +52,43 @@ abstract class FormattingFunction extends Function {
*/
predicate isWideCharDefault() { none() }
/**
* Gets the default character type expected for `%s` by this function. Typically
* `char` or `wchar_t`.
*/
Type getDefaultCharType() {
result = stripTopLevelSpecifiersOnly(getParameter(getFormatParameterIndex()).getType().
getUnderlyingType().(PointerType).getBaseType())
}
/**
* Gets the non-default character type expected for `%S` by this function. Typically
* `wchar_t` or `char`. On some snapshots there may be multiple results where we can't tell
* which is correct for a particular function.
*/
Type getNonDefaultCharType() {
(
getDefaultCharType().getSize() = 1 and
result = getAFormatterWideTypeOrDefault()
) or (
getDefaultCharType().getSize() > 1 and
result instanceof PlainCharType
)
}
/**
* Gets the wide character type for this function. This is usually `wchar_t`. On some
* snapshots there may be multiple results where we can't tell which is correct for a
* particular function.
*/
Type getWideCharType() {
(
result = getDefaultCharType() or
result = getNonDefaultCharType()
) and
result.getSize() > 1
}
/**
* Gets the position at which the output parameter, if any, occurs.
*/

View File

@@ -1,5 +1,4 @@
| custom_printf.cpp:31:5:31:12 | call to myPrintf | Format expects 2 arguments but given 3 |
| custom_printf.cpp:44:2:44:7 | call to printf | Format expects 0 arguments but given 2 |
| macros.cpp:12:2:12:31 | call to printf | Format expects 2 arguments but given 3 |
| macros.cpp:16:2:16:30 | call to printf | Format expects 2 arguments but given 3 |
| test.c:7:2:7:7 | call to printf | Format expects 0 arguments but given 1 |

View File

@@ -1,5 +1,4 @@
| custom_printf.cpp:29:5:29:12 | call to myPrintf | Format expects 2 arguments but given 1 |
| custom_printf.cpp:45:2:45:7 | call to printf | Format expects 2 arguments but given 0 |
| macros.cpp:14:2:14:37 | call to printf | Format expects 4 arguments but given 3 |
| macros.cpp:21:2:21:36 | call to printf | Format expects 4 arguments but given 3 |
| test.c:9:2:9:7 | call to printf | Format expects 1 arguments but given 0 |

View File

@@ -41,6 +41,6 @@ void test_custom_printf2()
{
// notTheFormat format ...
printf(0, "%i %i", 100, 200); // GOOD
printf("", "%i %i", 100, 200); // GOOD [FALSE POSITIVE]
printf("%i %i", "" ); // GOOD [FALSE POSITIVE]
printf("", "%i %i", 100, 200); // GOOD
printf("%i %i", "" ); // GOOD
}

View File

@@ -0,0 +1,10 @@
| tests.cpp:18:15:18:22 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:19:15:19:22 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
| tests.cpp:25:17:25:23 | Hello | This argument should be of type 'wchar_t *' but is of type 'char *' |
| tests.cpp:26:17:26:24 | Hello | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
| tests.cpp:30:17:30:24 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:31:17:31:24 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |
| tests.cpp:33:36:33:42 | Hello | This argument should be of type 'char16_t *' but is of type 'char *' |
| tests.cpp:35:36:35:43 | Hello | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |
| tests.cpp:38:36:38:43 | Hello | This argument should be of type 'char *' but is of type 'char16_t *' |
| tests.cpp:39:36:39:43 | Hello | This argument should be of type 'char *' but is of type 'wchar_t *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,3 @@
| tests.cpp:8:5:8:10 | printf | char | char16_t, wchar_t | char16_t, wchar_t |
| tests.cpp:9:5:9:11 | wprintf | wchar_t | char | wchar_t |
| tests.cpp:10:5:10:12 | swprintf | char16_t | char | char16_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -0,0 +1,40 @@
/*
* Test for custom definitions of *wprintf using different types than the
* platform wide character type.
*/
typedef unsigned int size_t;
int printf(const char * format, ...);
int wprintf(const wchar_t * format, ...); // on wchar_t
int swprintf(char16_t * s, size_t n, const char16_t * format, ...); // on char16_t
#define BUF_SIZE (4096)
void tests() {
char16_t buffer[BUF_SIZE];
printf("%s", "Hello"); // GOOD
printf("%s", u"Hello"); // BAD: expecting char
printf("%s", L"Hello"); // BAD: expecting char
printf("%S", "Hello"); // BAD: expecting wchar_t or char16_t [NOT DETECTED]
printf("%S", u"Hello"); // GOOD
printf("%S", L"Hello"); // GOOD
wprintf(L"%s", "Hello"); // BAD: expecting wchar_t
wprintf(L"%s", u"Hello"); // BAD: expecting wchar_t
wprintf(L"%s", L"Hello"); // GOOD
wprintf(L"%S", "Hello"); // GOOD
wprintf(L"%S", u"Hello"); // BAD: expecting char
wprintf(L"%S", L"Hello"); // BAD: expecting char
swprintf(buffer, BUF_SIZE, u"%s", "Hello"); // BAD: expecting char16_t
swprintf(buffer, BUF_SIZE, u"%s", u"Hello"); // GOOD
swprintf(buffer, BUF_SIZE, u"%s", L"Hello"); // BAD: expecting char16_t
swprintf(buffer, BUF_SIZE, u"%S", "Hello"); // GOOD
swprintf(buffer, BUF_SIZE, u"%S", u"Hello"); // BAD: expecting char
swprintf(buffer, BUF_SIZE, u"%S", L"Hello"); // BAD: expecting char
}

View File

@@ -0,0 +1,2 @@
| tests_32.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |
| tests_64.cpp:14:16:14:23 | void_ptr | This argument should be of type 'long' but is of type 'void *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,17 @@
// semmle-extractor-options: --edg --target --edg linux_i686
/*
* Test for printf in a snapshot that contains multiple word/pointer sizes.
*/
int printf(const char * format, ...);
void test_32()
{
long l;
void *void_ptr;
printf("%li", l); // GOOD
printf("%li", void_ptr); // BAD
printf("%p", l); // BAD [NOT DETECTED]
printf("%p", void_ptr); // GOOD
}

View File

@@ -0,0 +1,17 @@
// semmle-extractor-options: --edg --target --edg linux_x86_64
/*
* Test for printf in a snapshot that contains multiple word/pointer sizes.
*/
int printf(const char * format, ...);
void test_64()
{
long l;
void *void_ptr;
printf("%li", l); // GOOD
printf("%li", void_ptr); // BAD
printf("%p", l); // BAD [NOT DETECTED]
printf("%p", void_ptr); // GOOD
}

View File

@@ -0,0 +1,2 @@
| printf.cpp:45:29:45:35 | test | This argument should be of type 'char *' but is of type 'char16_t *' |
| printf.cpp:52:29:52:35 | test | This argument should be of type 'char16_t *' but is of type 'wchar_t *' |

View File

@@ -0,0 +1 @@
Likely Bugs/Format/WrongTypeFormatArguments.ql

View File

@@ -0,0 +1,2 @@
| printf.cpp:15:5:15:12 | swprintf | char16_t | char | char16_t |
| printf.cpp:26:5:26:11 | sprintf | char | char16_t | char16_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -0,0 +1,53 @@
/*
* Test for custom definitions of *wprintf using different types than the
* platform wide character type.
*/
#define WCHAR char16_t
typedef void *va_list;
#define va_start(va, other)
#define va_end(args)
int vswprintf(WCHAR *dest, WCHAR *format, va_list args) {
return 0;
}
int swprintf(WCHAR *dest, WCHAR *format, ...) {
va_list args;
va_start(args, format);
int ret = vswprintf(dest, format, args);
va_end(args);
return ret;
}
int sprintf(char *dest, char *format, ...);
// ---
void test1() {
WCHAR string[20];
swprintf(string, u"test %s", u"test"); // GOOD
}
void test2() {
char string[20];
sprintf(string, "test %S", u"test"); // GOOD
}
void test3() {
char string[20];
sprintf(string, "test %s", u"test"); // BAD: `char16_t` string parameter read as `char` string
}
void test4() {
char string[20];
sprintf(string, "test %S", L"test"); // BAD: `wchar_t` string parameter read as `char16_t` string
}

View File

@@ -0,0 +1,4 @@
| common.h:12:12:12:17 | printf | char | wchar_t | wchar_t |
| format.h:4:13:4:17 | error | char | wchar_t | wchar_t |
| real_world.h:8:12:8:18 | fprintf | char | wchar_t | wchar_t |
| real_world.h:33:6:33:12 | msg_out | char | wchar_t | wchar_t |

View File

@@ -0,0 +1,8 @@
import cpp
from FormattingFunction f
select
f,
concat(f.getDefaultCharType().toString(), ", "),
concat(f.getNonDefaultCharType().toString(), ", "),
concat(f.getWideCharType().toString(), ", ")

View File

@@ -17,7 +17,7 @@
| printf1.h:74:19:74:22 | C_ST | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:75:19:75:28 | sizeof(<expr>) | This argument should be of type 'ssize_t' but is of type 'unsigned long long' |
| printf1.h:84:23:84:35 | ... - ... | This argument should be of type 'ssize_t' but is of type 'long long' |
| real_world.h:46:36:46:43 | filename | This argument should be of type 'wchar_t *' but is of type 'char16_t *' |
| printf1.h:130:18:130:18 | 0 | This argument should be of type 'void *' but is of type 'int' |
| real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
| real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
| real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |

View File

@@ -101,3 +101,31 @@ void fun1(unsigned char* a, unsigned char* b) {
printf("%td\n", pdt); // GOOD
printf("%td\n", a-b); // GOOD
}
typedef wchar_t WCHAR_T; // WCHAR_T -> wchar_t -> int
typedef int MYCHAR; // MYCHAR -> int (notably not via the wchar_t typedef)
void fun2() {
wchar_t *myString1;
WCHAR_T *myString2;
int *myString3;
MYCHAR *myString4;
printf("%S", myString1); // GOOD
printf("%S", myString2); // GOOD
printf("%S", myString3); // GOOD
printf("%S", myString4); // GOOD
}
typedef void *VOIDPTR;
typedef int (*FUNPTR)(int);
void fun3(void *p1, VOIDPTR p2, FUNPTR p3, char *p4)
{
printf("%p\n", p1); // GOOD
printf("%p\n", p2); // GOOD
printf("%p\n", p3); // GOOD
printf("%p\n", p4); // GOOD
printf("%p\n", p4 + 1); // GOOD
printf("%p\n", 0); // GOOD [FALSE POSITIVE]
}