[CPP-340] Refinements to FutileParams.ql etc. #1136

zlaski-semmle · 2019-03-19T23:09:37Z

This is a continuation of #790

Update QHELP file, test and test results.

()-declared functions.

jbj

I think this would be better as a new query instead of being a rewrite of FutileParams.ql. It should have @severity error instead of warning because it's more likely to be a security risk to pass too few arguments than too many. The qhelp can also be less abstract if it doesn't have to explain both the cases of too few and too many arguments.

cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql

cpp/ql/test/query-tests/Likely Bugs/Likely Typos/FutileParams/test.c

cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.qhelp

cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql

jbj · 2019-03-20T10:00:04Z

I ran this query on 68 projects and got these results: https://lgtm.com/query/8608284463077038011/. Please use them to improve the query/queries.

jbj · 2019-03-20T10:01:47Z

If you make a new query instead of rewriting FutileParams.ql, you can let the two queries share the same directory of test files. That makes it easier to ensure there's no case where they both give alerts for the same bug.

(2) too few arguments and (3) too many arguments. Create new 'UnderspecifiedFunction' folders for both queries and tests.

zlaski-semmle · 2019-03-21T02:59:49Z

Split the original query into 3 different ones -- mismatched arguments, too few arguments, too many arguments. LGTM validation of queries is ongoing.

Need assistance in:
(1) qhelp, esp. the <references> section - what do we put there?
(2) the top portion of each QL query -- @precision, @tags
(3) for MismatchedFunctionArguments.ql, the correct syntax of the select clause: I want to print out argument and parameter name in addition to the function name.

zlaski-semmle · 2019-03-21T03:41:28Z

Hitting false positives on LGTM with MistypedFunctionArguments.ql. Need to figure out how to determine compatibility of types rather than equality.

jbj · 2019-03-21T09:13:22Z

For the query metadata and qhelp, it helps to have run the query on many LGTM projects to get an idea of what the typical mistakes are and what the typical FPs look like. The MistypedFunctionArguments.ql query is going to be difficult to get right, and it might turn out we just can't get the FP rate low enough. If we might end up scrapping it, there's no need to invest time in qhelp at this point.

The definition of compatible types in C is https://en.cppreference.com/w/c/language/type#Compatible_types. It has some quirks that make it difficult to reason about in QL. We can't just write predicate areTypesCompatible(Type t1, Type t2) { ... } in QL because that would enumerate all pairs of compatible types, which would take too much time and space.

I have some ideas for how to compute compatible types accurately in QL, but let's do something much simpler for the initial query:

// This predicate doesn't necessarily have to exist, but if it does exist
// then it must be inline to make sure we don't enumerate all pairs of
// compatible types.
// Its body could also just be hand-inlined where it's used.
pragma[inline]
predicate mayBeCompatible(Type t1, Type t2) {
  getCanonical(t1) = getCanonical(t2)
}

The getCanonical predicate should first call getUnspecifiedType on its argument and then map the result along these lines:

Primitive types (int, unsigned char, float complex, ...) map to themselves.
Enumerations map to their underlying type.
Everything else maps to VoidType.

That means we'll believe a struct foo is compatible with a void *, but this should only lead to false negatives and not false positives. That means we can get the query off the ground and postpone the complicated bits to a later PR.

geoffw0 · 2019-03-21T09:17:35Z

Here's the wiki page on query severity and precision: https://wiki.semmle.com/pages/viewpage.action?spaceKey=IN&title=Query+classification+and+display. Note in particular the tables at the bottom, since these two values are used to determine whether a query is available / displayed by default on LGTM. Queries that are displayed by default on LGTM must have a low false positive rate.

And here's the wiki page about query names and descriptions: https://wiki.semmle.com/display/IN/Writing+queries+and+query+help#Writingqueriesandqueryhelp-Querytagstags

For @tags the main ones are correctness, maintainability and security, the latter of which should be accompanies by the appropriate external/cwe/cwe-xyz tag.

geoffw0 · 2019-03-21T09:22:01Z

cpp/ql/src/Likely Bugs/Likely Typos/FutileParams.ql

-  and fc.getNumberOfArguments() != 0
-  and not f instanceof BuiltInFunction
-  and exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() | not fde.isImplicit())
-select fc, "This call has arguments, but $@ is not declared with any parameters.", f, f.toString()


You will need to add a change note so that any customers who are using this query can figure out what they will need to run instead.

... ah, it looks like the query is new and may not have been in any releases yet. In that case, all that is needed is a change note for the new queries.

This file is no longer used; look at cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/*.ql instead. Sorry for moving things around like that, I just wanted to create a more modular folder structure.

By changelog, do you mean *.qhelp. Also, what do you mean by "what they will need to run instead"?

The change notes are here: https://github.com/Semmle/ql/tree/master/change-notes

A change note is required for any change that might affect customers, which includes moving an existing query.

I looked at the changelogs at the URL you provided, but don't see any references to query locations (by which I'm assuming you mean locations of the .ql files). I do see references to query IDs, though...

zlaski-semmle · 2019-03-21T14:26:13Z

@jbj, thanks for the input. It's a bit surprising that the QL cannot handle this reliably, but I will do as you suggest.

zlaski-semmle · 2019-03-21T14:28:46Z

@geoffw0, where does one get the external/cwe/cwe-xyz tag? It would especially apply to the TooFewArguments.ql query.

geoffw0 · 2019-03-21T16:07:15Z

@geoffw0, where does one get the external/cwe/cwe-xyz tag? It would especially apply to the TooFewArguments.ql query.

The CWE numbers come from this site: https://cwe.mitre.org/data/definitions/1000.html (that particular page shows them in a tree-like structure; some queries are tagged with a parent or child of what might be the most accurate CWE describing them, for historic reasons.

If you're not aware of a particular CWE, I believe it's acceptable to just use the security tag alone (contrary to how I described it earlier, sorry).

C promotion rules. The following issues are now flagged: (1) passing a larger type than the receiver can accept (e.g., long long -> int) (2) passing a type of different signedness than the parameter specified.

zlaski-semmle · 2019-03-25T02:52:23Z

Just checked in the following:

[CPP-340] Refine the test query for mismatching args/params by applying C promotion rules. The following issues are now flagged: (1) passing a larger type than the receiver can accept (e.g., long long -> int) (2) passing a type of different signedness than the parameter specified.

I'm seeing very few hits running the 68 projects that @jbj specified, and the hits all look like legitimate issues. A possible question to resolve is whether case (2) should be relaxed or eliminated.

jbj · 2019-03-25T12:39:53Z

For (2), I just tried to test what a C compiler might warn about:

int foo(int x) {
  return x;
}

int callFoo(unsigned int y) {
  return foo(y);
}

I don't get any warnings from this, either with GCC or Clang at -Wall -Wpedantic. If a pedantic C compiler doesn't warn about it in the case where a prototype exists, I don't see why we should warn about it when there's no prototype.

jbj

I haven't finished reviewing, but I've written enough comments that I'll take a break here and let you reply.

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/MistypedFunctionArguments.qhelp

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooFewArguments.qhelp

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooManyArguments.qhelp

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/MistypedFunctionArguments.ql

jbj · 2019-03-25T13:48:17Z

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/MistypedFunctionArguments.ql

+    fde.getNumberOfParameters() = 0
+  ) and
+  // Parameter p and its corresponding call argument must have mismatched types
+  not argMayBePromoted(fc.getArgument(p.getIndex()), p)


I think fc.getArgument(...) should be fc.getArgument(...).getFullyConverted() to make sure you look at the type after all the implicit conversions. Also the replacement for "of type $@" but not in the replacement for "argument $@".

This should first of all fix the NumPy FP I mentioned above because the conversion from size_t to double will be accounted for. I hope it'll also account for the promotions like char to int that must happen when calling functions without prototype, meaning you can remove those from argTypeMayBePromoted.

But converting size_t to double is lossy and hence not a FP (in my opinion).

Yes, that conversion is technically lossy (for values over 2^53), but (1) that has nothing to do with whether the function has a prototype, and (2) if we warn about every lossy implicit conversion, the more severe results will drown in the noise.

We've recently added a query to check for particularly easy-to-miss cases of lossy conversion from floating point to integer: https://lgtm.com/rules/1507250565973/. The results from that query look valid in a technical sense, but we still haven't made the query visible by default. The results also need to be true positives under our strong definition: that the developer thinks it's worth fixing.

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooFewArguments.c

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooManyArguments.c

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/MistypedFunctionArguments.c

jbj · 2019-03-25T13:53:09Z

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/MistypedFunctionArguments.c

+
+void calls() {
+	three_arguments(1, 2, 3); // GOOD
+	three_arguments(1.0f, 2, 3.0f); // BAD: arguments 1 and 3 do not match parameters


I don't think this is BAD. The declaration of three_arguments has a prototype, so the floats will be converted to int semantically -- rounded down, not re-interpreted as a nonsense bit pattern.

When compiling the following file:

//void three_arguments(); void three_arguments(int x, char y, int *z); int var; double d = 3732.70e13L; extern unsigned long long *ull; void calls() { three_arguments(1, 2, &var); // GOOD three_arguments(1U, 2U, &var); // BAD: arguments 1, 2 and 3 do not match parameters three_arguments(1.0f, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(1.0L, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(d, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(2ULL, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(*ull, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(&ull, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(&var, 2, &var); // BAD: arguments 1 and 3 do not match parameters three_arguments(var, 2, 3.0f); // BAD: arguments 1 and 3 do not match parameters }

Both gcc and clang only issue a diagnostic on the last 3 invocations of three_arguments, and a warning at that. This even with --pedantic. All of these involve pointers.

So the question is, which arguments shall we deem compatible when constructing MistypedFunctionArguments.ql? As I've made clear in this PR thread, I believe that all but the first invocation of three_arguments are bad (and the query in its present state mostly reflects this), but it seems that my position is becoming more untenable by the day. Should we just warn on pointer-related mismatches? What do people think?

jbj · 2019-03-25T14:14:10Z

Here are the results of the two new queries on 68 projects:

Call to a function with one or more incompatible arguments.
Call to function with fewer arguments than declared parameters.

@zlaski-semmle, please have a look through them and test cases inspired by the FPs.

I'm pleased to see that there are some good results. The ones I noticed were cases where long or size_t is passed where int is expected. I don't see how those could be to be miscompiled in practice on little-endian 32-bit or 64-bit architectures, but I haven't checked.

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooManyArguments.ql

compilers do. Various integral and floating-point types are treated as mutually implicitly convertible. Remaining warnings deal with misuse of pointer and array types.

zlaski-semmle · 2019-03-30T03:41:47Z

Just pushed more bits, this time trying to mimic what C compilers (in the absence of a ()-prototype) warn about or not. The warning does not appear in any of the 68 LGTM projects under analysis.

zlaski-semmle · 2019-04-01T18:32:50Z

Per @dave-bartolomeo 's suggestion to see if function params are always promoted, e.g., from float to double, I generated assembly for the following program:

extern float foo(float parm);

float bar() {
  return foo(3.f);
}

The following assembly is produced (roughly the same for gcc and clang):

   :
  .LCPI0_0:
	.long	1077936128              # float 3
	.text
	.globl	bar
	.p2align	4, 0x90
	.type	bar,@function
bar:                                    # @bar
	.cfi_startproc
# %bb.0:
	pushq	%rbp
	.cfi_def_cfa_offset 16
	.cfi_offset %rbp, -16
	movq	%rsp, %rbp
	.cfi_def_cfa_register %rbp
	movss	.LCPI0_0(%rip), %xmm0   # xmm0 = mem[0],zero,zero,zero
	callq	foo
	popq	%rbp
	retq
   :

So we see that floats are left alone.

geoffw0 · 2019-04-17T08:11:08Z

So static_assert is implicit in C++ (just like sizeof and the like), but is completely undefined in C, so I can't really check it for implicitness.

When I said 'implicit' I meant a very specific thing - a C-only [mis-]feature where a function that has not been forward declared is assumed to take no parameters and return an int:
https://scc.ustc.edu.cn/zlsc/sugon/intel/ssadiag_docs/pt_reference/references/sc_not_fun_forword.htm

static_assert and strptime were implicitly declared in the cases we discussed because the relevant header file containing a forward declaration was not included - but it's easy to do this with any function in C.

The extractor creates a function declaration at the call site when this happens, you should be able to exclude them using FunctionDeclarationEntry.isImplicit().

zlaski-semmle · 2019-04-17T17:50:32Z

You're right, I was misusing the term implicit (i.e., treating it as synonymous with built-in). Thing is, @jbj and myself agreed that implicit declarations (in the correct meaning of the term) be treated the same way as ()-declarations, and so I cannot exclude FunctionDeclarationEntry.isImplicit() entries.

I think the best way to move forward is to use white-listing, which is what my latest git push implements.

geoffw0 · 2019-04-18T10:04:52Z

@jbj and myself agreed that implicit declarations (in the correct meaning of the term) be treated the same way as ()-declarations, and so I cannot exclude FunctionDeclarationEntry.isImplicit() entries.

I partially disagree with this. I agree that they are equivalent from a correctness point of view, but the cause is slightly different and the message This call has more arguments than required by XYZ is misleading in the case of an implicit FDE. I think the way forward is not to whitelist but to implement a different message - or use a separate query entirely - and say something like This call to an implicitly declared function has arguments. with a suggestion in the documentation that you're probably missing an include with the proper declaration in it.

zlaski-semmle · 2019-04-18T17:27:16Z

@geoffw0, I totally agree that the message is misleading in case of implicit (and only implicit) declarations. So I'm off to creating yet another query called ArgumentsToImplicit.ql (alternative naming ideas welcome) that will deal with the implicit case, and modify TooManyArguments to deal with the non-implicit case. The latter will be a bit tricky, since an implicit definition will be created at each call site (I think).

As for white-listing, I think this functionality has to stay, since we know the signature of system functions even if their prototypes are missing. Look at the strptime() example encountered during the 68-project run.

…y with implicitly declared functions. TooManyArguments.ql will now deal with explicitly declared/prototyped functions.

zlaski-semmle · 2019-04-19T01:17:07Z

Ok, I've separated the handling of implicit functions into a separate query. Running LGTM suite now.

zlaski-semmle · 2019-04-19T02:12:36Z

ArgumentsToImplicit.ql: https://lgtm.com/query/7515296003062208953/
TooManyArguments.ql: https://lgtm.com/query/3790340098144082178/

zlaski-semmle · 2019-04-22T16:53:22Z

Per this morning's discussion with @dave-bartolomeo, here is my source-level analysis of Enlightenment/efl:

First off, we have two forward-declarations of win_add():

src/opt/src/src/tests/elementary/efl_ui_suite.h:44:Eo *win_add();
src/opt/src/src/tests/elementary/elm_suite.h:101:Evas_Object *win_add();

Then, we have a ton of calls to win_add(), like so:

src/opt/src/src/tests/elementary/elm_code_test_widget_selection.c:360:   win = win_add(NULL, "code", ELM_WIN_BASIC);
src/opt/src/src/tests/elementary/efl_ui_test_image_zoomable.c:16:   win = win_add(NULL, "photocam", EFL_UI_WIN_TYPE_BASIC);
     :

Finally we have a definition:

src/opt/src/src/tests/elementary/suite_helpers.c:220:
Evas_Object *
win_add()
{
   if (getpid() != main_pid)
     {
        if (global_win) return global_win;
     }
   return _elm_suite_win_create();
}

I'm not sure if this qualifies as K&R style (unlike, say, win_add(void)), but in any event the TooManyArguments.ql query flags the problem correctly: win_add() is called with arguments with no corresponding formal parameters. While this is technically correct C, the point of this query (and the 3 others) is to detect code that is potentially problematic.

dave-bartolomeo · 2019-04-22T22:06:13Z

@zlaski-semmle That example isn't quite what I thought it was. I thought we were talking about:

/* a.c */
int foo();
int bar() {
  return foo(1, 2);
}
/* b.c */
int foo(x, y)
  int x;
  int y;
{
  return x + y;
}

In your example, the number of arguments at the call site does not match the number of parameters in the definition, which is indeed undefined behavior. We should report an alert for such a case.

zlaski-semmle · 2019-04-22T22:12:13Z

Do you guys see any other issues with my fixes? If not, can you please merge them in?

geoffw0 · 2019-04-23T09:14:59Z

As for white-listing, I think this functionality has to stay, since we know the signature of system functions even if their prototypes are missing. Look at the strptime() example encountered during the 68-project run.

I still don't understand the need for the whitelist. The results for static_assert and strptime would now appear under ArgumentsToImplicit.ql where they are correct, right?

zlaski-semmle · 2019-04-24T03:42:26Z

The whole point of the white list is that static_assert and friends do not appear in the queries (since we know their signature in the real world). Or am I misunderstanding something?

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql

jbj · 2019-04-24T09:01:06Z

cpp/ql/src/Likely Bugs/Underspecified Functions/ArgumentsToImplicit.ql

+ * @name Call with arguments to an implicitly declared function
+ * @description A function call passed arguments even though the
+ *              function in question is only implicitly declared (and
+ *              hence accepting no arguments).  This may indicate


I disagree with your statement that an implicitly declared function accepts no arguments. In the interest of getting this PR merged, I suggest removing this query. It should be independent enough to go in a separate PR.

I thought that we agreed that implicitly declared and ()-declared functions should be treated similarly. Please elaborate.

I started to break off ArgumentsToImplicit.ql, but then realized that doing so would break our test case. I believe that *.qlref files residing in the same folder should be tested together, and that there is no way for the tests to be separated in some way. Am I wrong?

I think it would be simpler if ArgumentsToImplicit.ql were reviewed with the other queries.

So I think the issue here is quite simple: If we agree that passing arguments to implicitly-declared functions is something to inform the user about, then we keep the ArgumentsToImplicit.ql query. Otherwise, we simply get rid of it.

Calling an implicitly-declared function is dangerous, but I disagree that passing an argument makes it any more dangerous. The compiler will have already warned about the implicit declaration, so I see no urgency to add a query for it.

I don't understand the problem with qlref files you describe. With qltest, all source files (c, cpp) in a directory get extracted and imported into a single database, and then each ql or qlref file is evaluated on that database. These evaluations are independent and produce separate expected files.

Sorry, got confused by the fact that each .expected file contains results for all C/C++ files in the folder.

If you don't believe we need to warn about parameters being passed to implicitly-declared function, then I'll simply remove ArgumentsToImplicit.ql.

jbj · 2019-04-24T11:03:27Z

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

+// then it must be inline to make sure we don't enumerate all pairs of
+// compatible types.
+// Its body could also just be hand-inlined where it's used.
+pragma[inline]


This predicate has changed enough that its comment is no longer relevant. Since it's only called from one place, I suggest inlining it by hand, i.e., removing it.

I agree that the comment is not terribly useful. However, I prefer to separate things into predicates as they improve code readability. Is there a run-time performance hit for doing so?

There's almost no run-time difference between using pragma[inline] and hand-inlining a predicate.

In any case, the comment needs to go. It doesn't describe this predicate.

jbj · 2019-04-24T11:40:12Z

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

+    parm.(PointerType).getBaseType().getUnspecifiedType())
+  or
+  pointerArgTypeMayBeUsed(arg.(ArrayType).getBaseType().getUnspecifiedType(),
+    parm.(ArrayType).getBaseType().getUnspecifiedType())


I thought about it some more, and now I think you do need the arg.(ArrayType) cases in order to handle the rarely-used "pointer to array" types (declared like int (*parameterName)[2]).

But hang on, where is the recursion that would enable this? If you allow passing a int * to a unsigned int * argument, shouldn't you also allow passing an int ** to a unsigned int ** argument? But recursive predicates can't be inline, so you can't support this without some serious restructuring for the sake of performance.

To start with, please make sure there is test coverage for "pointer to array" and "pointer to pointer" to compatible-but-not-identical base types.

jbj · 2019-04-24T11:55:15Z

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql

+  not f.isVarargs() and
+  hasZeroParamDecl(f) and
+  isCompiledAsC(f) and
+  not isWhitelisted(f) and


This manual whitelist is not a maintainable solution. Do I understand it correctly that this query will give an alert even in cases where we can't see the definition of the function? I don't think we can justify that since we have no idea what the correct argument count is. Why not require that the definition is in the snapshot? For C code, I think that's effectively exists(f.getBlock()).

The purpose of the whitelist is to not give an alert for these functions, even though we may not see their prototype or definition. Also, I'm not sure what to make of your exists(f.getBlock()). Where would you use it and what would it accomplish?

As far as your pointer-to-array request, I propose that we simply "unroll" the indirection one extra time, without recursion. While it is certainly possible to write something like int ***i;, I don't believe we would encounter it in practice.

Will rewrite using recursion, per 1-on-1 discussion.

Per our (@jbj and myself) Slack discussion, we will keep the non-recursive, two-indirection solution and lower the query precision to medium.

We agreed over Google Meet to replace the whitelist with exists(f.getBlock()), right?

jbj · 2019-04-24T11:57:28Z

MistypedFunctionArguments.ql: https://lgtm.com/query/5884827591141130989/

Where did the good results in zlib go? The original query run found cases where an unsigned long is passed to an unsigned int parameter.

…to pointers and pointers to arrays.

zlaski-semmle · 2019-04-25T21:14:28Z

The "good" results in madler/zlib went away due to argument conversion rather than argument promotion. Note the use of the getConvertedType() predicate in the type matching logic.

Reduce MistypedFunctionArguments.ql precision to `medium`.

change-notes/1.21/analysis-cpp.md

jbj · 2019-04-29T11:49:03Z

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql

+  not f.isVarargs() and
+  hasZeroParamDecl(f) and
+  isCompiledAsC(f) and
+  not isWhitelisted(f) and


We agreed over Google Meet to replace the whitelist with exists(f.getBlock()), right?

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

Bring back entry accidentally deleted during previous merge.

files to be regenerated.

zlaski-semmle · 2019-04-29T22:37:59Z

Using f.getBlock() caused TooManyArguments.ql to produce slightly different test results. I have added a new scenario to test.c: a ()-declared function is called with arguments and subsequently ()-defined. This was the pattern we were seeing with Enlightenment/efl but were not testing for explicitly.

These changes to test.c required the re-generation of all three .expected files, unfortunately.

jbj · 2019-04-30T11:05:00Z

We've lost many results over the course of these reviews, but follow-up work will need to go in fresh new PRs since this PR discussion is far too long.

jbj · 2019-04-30T11:05:34Z

LGTM apart from the comment I just made on TooManyArguments.expected.

zlaski-semmle · 2019-04-30T20:20:41Z

Good catch. Both lines 29 and 31 of test.c should have been marked as // GOOD (since we agreed that unspecified and ()-specified -- though not defined -- function are to be allowed arguments).

zlaski-semmle added 3 commits March 19, 2019 16:00

[CPP-340] Create QL query for function call argument count mismatches.

0c350dc

Update QHELP file, test and test results.

Refine QL query by requiring that a ()-declaration be present.

5d8b84c

[CPP-340] Re-work QL query; treat undeclared C functions the same way as

2def0ee

()-declared functions.

zlaski-semmle requested a review from a team as a code owner March 19, 2019 23:09

zlaski-semmle mentioned this pull request Mar 19, 2019

C++: new query for futile arguments to C functions #790

Merged

jbj reviewed Mar 20, 2019

View reviewed changes

[CPP-340] Create three QL queries: (1) mismatched argument types,

5a092d0

(2) too few arguments and (3) too many arguments. Create new 'UnderspecifiedFunction' folders for both queries and tests.

geoffw0 reviewed Mar 21, 2019

View reviewed changes

[CPP-340] Refine the test query for mismatching args/params by applying

29af56d

C promotion rules. The following issues are now flagged: (1) passing a larger type than the receiver can accept (e.g., long long -> int) (2) passing a type of different signedness than the parameter specified.

jbj reviewed Mar 25, 2019

View reviewed changes

jbj added the C++ label Mar 27, 2019

jbj reviewed Mar 27, 2019

View reviewed changes

cpp/ql/src/Likely Bugs/UnderspecifiedFunctions/TooManyArguments.ql Outdated Show resolved Hide resolved

zlaski-semmle added 4 commits March 29, 2019 20:19

[CPP-340] When warning about mismatched parameters, follow what C

cb5bbd2

compilers do. Various integral and floating-point types are treated as mutually implicitly convertible. Remaining warnings deal with misuse of pointer and array types.

[CPP-340] Refer to C coding standard, not C++.

2ea9f81

[CPP-340] cpp/too-many-arguments should remain as cpp/futile-params.

59a54df

[CPP-340] Fix TooFewArguments.c to actually provide a ()-prototype.

8a653b9

[CPP-340] Add a fourth query, ArgumentsToImplicit.ql, to deal strictl…

62b030d

…y with implicitly declared functions. TooManyArguments.ql will now deal with explicitly declared/prototyped functions.

[CPP-340] Minor formatting tweaks

36b2c14

jbj reviewed Apr 24, 2019

View reviewed changes

[CPP-340] For MistypedFunctionArguments.ql, add support for pointers …

ac58bdf

…to pointers and pointers to arrays.

[CPP-340] Delete ArgumentsToImplicit.ql and associated files.

4a760b1

Reduce MistypedFunctionArguments.ql precision to `medium`.

jbj reviewed Apr 29, 2019

View reviewed changes

zlaski-semmle added 3 commits April 29, 2019 09:58

[CPP-340] Replace whitelist with f.getBlock() test. Fix doc comment.

a0cfe82

[CPP-340] Fix end-of-line formatting for our change notes.

d146967

Bring back entry accidentally deleted during previous merge.

[CPP-340] Add new test cases to test.c; this required the .expected

be77eb7

files to be regenerated.

[CPP-340] Adjust annotations in test.c file.

17066cf

jbj approved these changes May 1, 2019

View reviewed changes

jbj merged commit 54091e8 into github:master May 1, 2019

felicitymay mentioned this pull request Jun 7, 2019

QL1.21: Finalize change notes for C/C++ #1413

Merged

[CPP-340] Refinements to FutileParams.ql etc. #1136

[CPP-340] Refinements to FutileParams.ql etc. #1136

Uh oh!

Conversation

zlaski-semmle commented Mar 19, 2019

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbj commented Mar 20, 2019

Uh oh!

jbj commented Mar 20, 2019

Uh oh!

zlaski-semmle commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlaski-semmle commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbj commented Mar 21, 2019

Uh oh!

geoffw0 commented Mar 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zlaski-semmle commented Mar 21, 2019

Uh oh!

zlaski-semmle commented Mar 21, 2019

Uh oh!

geoffw0 commented Mar 21, 2019

Uh oh!

zlaski-semmle commented Mar 25, 2019

Uh oh!

jbj commented Mar 25, 2019

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbj commented Mar 25, 2019

Uh oh!

Uh oh!

zlaski-semmle commented Mar 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlaski-semmle commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zlaski-semmle commented Mar 21, 2019 •

edited

Loading

zlaski-semmle commented Mar 21, 2019 •

edited

Loading

zlaski-semmle commented Mar 30, 2019 •

edited

Loading

zlaski-semmle commented Apr 1, 2019 •

edited

Loading