690432 - Logging.h passes a string directly to printf

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: 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]]

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

(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)]

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]

(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]]

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)]

https://hg.mozilla.org/mozilla-central/rev/719cb2ed1bf6