493996 - Get an analysis to check for the right number of arguments to nsPrintfCString

Status: RESOLVED INCOMPLETE

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

over in bug 493602, we had a UMR issue because we didn't pass enough arguments to nsPrintfCString.  Functions like printf give a compiler error if you mess them up, but I don't think we can do that with nsPrintfCString.  We will have UMR issues though if we do this, so we should check for it.

Comment 1

[Benjamin Smedberg]

As long as you pass in a static string, this doesn't sound too hard... let me check a couple things.

Comment 2

[Shawn Wilsher :sdwilsh]

Yeah, I didn't think it'd be too difficult, but it's a really easy mistake to make.  Felt like a good candidate for analysis! :)

Comment 3

[Benjamin Smedberg]

Attachment: Use GCCs builting -Wformat stuff, rev. 1

This uses GCC's builtin -Wformat code and __attribute__((format(printf,f,a)))

This might give us some false-positives because NSPR format codes don't match up with libc format codes. But it's probably better than ignoring the problem altogether. I'll look at doing a better analysis also, because I'd really like to suppress the "format '%p' expects type 'void*', but argument 2 has type 'nsIDOMNode*'" warnings, which are stupid.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 378655 [details] [diff] [review]
Use GCCs builting -Wformat stuff, rev. 1

r=dbaron, but maybe add a source code comment about the possibility of mismatches due to differences between NSPR printf format and C printf format?  (Maybe the macro should even have NSPR in the name and be documented as really implying the NSPR printf format?)

Comment 5

[Benjamin Smedberg]

Attachment: Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1

Comment 6

[[e-mail]]

Comment on attachment 388700 [details] [diff] [review]
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1

I'm not familiar enough with treehydra to review the script yet, but can probably take a stab soonish, if you don't mind waiting...

Comment 7

[Benjamin Smedberg]

Ted won't be around until next week, but if it'll take longer than 2 weeks maybe Taras can look it through. It comes with tests so I'm not that worried about the code, just reviewing for style and readability.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 388700 [details] [diff] [review]
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1

+ * @param ifmt The 1-based index of the format.
+ * @param iargs The 1-based index of the varargs which correspond to the format.

Any particular reason these are 1-based indexes? Seems confusing to me.

r=me on the build/NSPR code bits.

Comment 9

[Benjamin Smedberg]

The 1-based indexes are for consistency with GCC __attribute__((format(__printf__)))

Comment 10

[[e-mail]]

Comment on attachment 388700 [details] [diff] [review]
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1

>diff --git a/nsprpub/pr/tests/prprf-analysis.js b/nsprpub/pr/tests/prprf-analysis.js

>+function process_tree_decl(d)
>+{
>+  let decl = dehydra_convert(d);

Just FYI, I've run into (what I assume is) a bug in dehydra_convert() where it fails. Compiling jsapi.cpp and calling dehydra_convert() from process_tree_type(), it ran through 2 gigs of memory and then failed (with spidermonkey saying it was out of stack space). Not sure if you've hit that, but it might bite people.

>+function process_tree(fdecl)
>+{

>+        function getLocation(skiptop) {
>+          if (!skiptop) {
>+            let loc = location_of(t);
>+            if (loc !== undefined)

You don't call getLocation() with skiptop=true. Also, is |loc !== undefined| intentional?

>+          
>+          for (let i = stack.length - 1; i >= 0; --i) {
>+            let loc = location_of(stack[i]);
>+            if (loc !== undefined)

Same here.

>+        let callee = call_function_decl(t);
>+        if (!callee)
>+          return;

So I'm guessing we don't care about virtual printf-ish functions?

>+          let arr = t.operands()[2 + ifmt].tree_check(ADDR_EXPR).
>+            operands()[0].tree_check(ARRAY_REF);
>+          let idx = TREE_INT_CST_LOW(arr.operands()[1].tree_check(INTEGER_CST));

Nice, I didn't know about tree_check.

r=me, was fun to read!

Comment 11

[Benjamin Smedberg]

Nobody is using dehydra now. If somebody wants to pick this up, please reopen (and consider doing this as clang analysis). Otherwise we'll just leave this INCOMPLETE.