Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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
5 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

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