Last Comment Bug 493996 - Get an analysis to check for the right number of arguments to nsPrintfCString
: Get an analysis to check for the right number of arguments to nsPrintfCString
Status: RESOLVED INCOMPLETE
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: ---
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
Depends on: 494960
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-20 11:17 PDT by Shawn Wilsher :sdwilsh
Modified: 2013-01-17 07:47 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use GCCs builting -Wformat stuff, rev. 1 (1.72 KB, patch)
2009-05-20 12:01 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
dbaron: review+
Details | Diff | Splinter Review
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1 (84.18 KB, patch)
2009-07-15 07:49 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
ted: review+
dwitte: review+
Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2009-05-20 11:17:52 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-05-20 11:19:49 PDT
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 2009-05-20 11:27:48 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-05-20 12:01:58 PDT
Created attachment 378655 [details] [diff] [review]
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+2 (review requests must explain patch) 2009-05-20 12:51:25 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-07-15 07:49:00 PDT
Created attachment 388700 [details] [diff] [review]
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1
Comment 6 dwitte@gmail.com 2009-07-15 12:09:35 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-07-15 13:29:07 PDT
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 Ted Mielczarek [:ted.mielczarek] 2009-08-06 09:55:21 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-08-06 10:07:09 PDT
The 1-based indexes are for consistency with GCC __attribute__((format(__printf__)))
Comment 10 dwitte@gmail.com 2009-08-06 14:14:58 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2013-01-17 07:47:52 PST
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.

Note You need to log in before you can comment on or make changes to this bug.