Created attachment 591707 [details] [diff] [review] js-formatstring.patch User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 Build ID: 20111223105406 Steps to reproduce: js -o "%n%n%n" Actual results: js segfaults Expected results: An error message should have been printed. I've attached a patch that corrects this and another instance where a string is used as a format string. Perhaps it'd be useful to add annotations to JS_ReportError and similar functions so that the compiler can warn when this happens.
Sorry, I typoed the command, it's actually: js -o "%x%x%x%n"
Nice find. The patch looks good. (Trivium: we usually use ".diff" as the suffix on patch filenames.) The next step is to request review. jwalden+bmo or :cdleary would be good choices for reviewer.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Fun fact: This bug was found and used by a lot of teams at the Mozilla CTF yesterday (http://mozillactf.org/), which is one of the only situations where a bug like this would have interesting uses and consequences :-)
Created attachment 591943 [details] [diff] [review] Add annotation to mfbt and use it. Adds the suggested annotation. Thanks for the tip, Ricky! We tend to be sloppy in the shell because isn't exposed to the web, so this should be helpful. :-)
Assignee: general → christopher.leary
Status: NEW → ASSIGNED
Attachment #591943 - Flags: review?
Attachment #591943 - Flags: review? → review?(dmandelin)
Attachment #591943 - Flags: review?(dmandelin) → review+
Note that in gcc, using printf attributes on a C++ member method has weird indexing behavior; that should be clarified somehow in the documentation: http://www.codemaestro.com/reviews/18
Will add that to the comment.
We seem to use %hs in a bunch of places in the code base, hoping that printf will treat that as a specifier for strings of 16b chars. Unfortunately, local testing in GCC shows that just doesn't work. man 3 printf shows that you can prefix the s conversion with an l character (i.e. "%ls") but that interprets it as a string of wchar_t, and I have no idea what wchar_t are defined to be in our code base and/or whether we should really rely on it.
Mass-reassigning cdleary's bugs to default. He won't work on any of them, anymore. I guess, at least. @cdleary: shout if you take issue with this.
Assignee: cdleary → general
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.