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

RESOLVED INCOMPLETE

Status

()

Core
Rewriting and Analysis
P3
normal
RESOLVED INCOMPLETE
8 years ago
4 years ago

People

(Reporter: sdwilsh, Assigned: bsmedberg)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
As long as you pass in a static string, this doesn't sound too hard... let me check a couple things.
(Reporter)

Comment 2

8 years ago
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! :)
(Assignee)

Comment 3

8 years ago
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.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #378655 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #378655 - Flags: review? → review?(dbaron)
Attachment #378655 - Flags: review?(dbaron) → review+
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?)
(Assignee)

Updated

8 years ago
Depends on: 494960
(Assignee)

Comment 5

8 years ago
Created attachment 388700 [details] [diff] [review]
Set up --enable-static-checking for NSPR and do PR_*printf analysis, rev. 1
Attachment #388700 - Flags: review?(ted.mielczarek)
Attachment #388700 - Flags: review?(dwitte)

Comment 6

8 years ago
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...
(Assignee)

Comment 7

8 years ago
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 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.
Attachment #388700 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 9

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

Comment 10

8 years ago
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!
Attachment #388700 - Flags: review?(dwitte) → review+
(Assignee)

Updated

8 years ago
Priority: -- → P3
(Assignee)

Comment 11

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.