As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
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 [:bsmedberg]
:
: Michael Layzell [:mystor]
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 [: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 [:bsmedberg]
ted: review+
dwitte: review+
Details | Diff | Splinter Review

Description User image 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 User image Benjamin Smedberg [: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 User image 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 User image Benjamin Smedberg [: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 User image David Baron :dbaron: ⌚️UTC-8 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 User image Benjamin Smedberg [: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 User image 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 User image Benjamin Smedberg [: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 User image 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 User image Benjamin Smedberg [:bsmedberg] 2009-08-06 10:07:09 PDT
The 1-based indexes are for consistency with GCC __attribute__((format(__printf__)))
Comment 10 User image 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 User image Benjamin Smedberg [: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.