Last Comment Bug 690432 - Logging.h passes a string directly to printf
: Logging.h passes a string directly to printf
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-29 10:59 PDT by Daniel Holbert [:dholbert]
Modified: 2011-09-29 20:44 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix: use printf format string (232 bytes, patch)
2011-09-29 10:59 PDT, Daniel Holbert [:dholbert]
bas: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-09-29 10:59:52 PDT
Created attachment 563468 [details] [diff] [review]
fix: use printf format string

Just noticed the attached patch in Ubuntu's patches for Firefox (from "apt-get source firefox").

The change is the following, in some GFX logging code:
> -    printf(aString.c_str());
> +    printf("%s", aString.c_str());

(If aString.c_str() were to contain printf-format-codes, I believe the current code would substitute in what it thinks are the corresponding arguments (random stack-memory), whereas the patched version wouldn't give the formatting codes any special handling & instead would just print them out verbatim.)

Since this is only used for internal logging, this probably isn't particularly worrisome; still, I think it's worth fixing, for correctness' sake.  Bas, any objections?

(not sure who to credit the patch to, but it may be trivial enough that it doesn't really matter)
Comment 1 Daniel Holbert [:dholbert] 2011-09-29 11:02:46 PDT
The patch was added to Ubuntu's source tree in this commit, btw:
http://bazaar.launchpad.net/~mozillateam/firefox/firefox-beta.head/revision/909
> * Fix "format not a string literal and no format arguments" error
>  - add debian/patches/printf-fix.patch
Comment 2 Daniel Holbert [:dholbert] 2011-09-29 11:03:54 PDT
(CC'ing Chris Coulson of Ubuntu, who made that commit.  Chris, assuming this gets review+, would you be the appropriate person to credit as patch-author?)
Comment 3 Bas Schouten (:bas.schouten) 2011-09-29 11:19:35 PDT
Comment on attachment 563468 [details] [diff] [review]
fix: use printf format string

Review of attachment 563468 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch! Sounds like the right thing to do to me!
Comment 4 Chris Coulson 2011-09-29 11:27:58 PDT
(In reply to Daniel Holbert [:dholbert] from comment #2)
> (CC'ing Chris Coulson of Ubuntu, who made that commit.  Chris, assuming this
> gets review+, would you be the appropriate person to credit as patch-author?)

Hi,

Yes, that's fine. I'm not sure why I didn't get around to reporting this myself, sorry about that :/

Note that we build with -Werror=format-security on Ubuntu as part of the build hardening, so the GCC warning that this bug triggers ends up as an error
Comment 5 Daniel Holbert [:dholbert] 2011-09-29 12:31:32 PDT
No worries on forgetting to file; thanks for the fix. :)

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/719cb2ed1bf6
Comment 6 Matt Brubeck (:mbrubeck) 2011-09-29 20:44:43 PDT
https://hg.mozilla.org/mozilla-central/rev/719cb2ed1bf6

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