The default bug view has changed. See this FAQ.

Logging.h passes a string directly to printf

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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)
Attachment #563468 - Flags: review?(bas.schouten)
(Assignee)

Comment 1

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

Comment 2

6 years ago
(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?)
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
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!
Attachment #563468 - Flags: review?(bas.schouten) → review+

Comment 4

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

Comment 5

6 years ago
No worries on forgetting to file; thanks for the fix. :)

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/719cb2ed1bf6
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/719cb2ed1bf6
Assignee: nobody → dholbert
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.