Closed Bug 1369560 Opened 3 years ago Closed 3 years ago
address potentially unsafe snprintf usage in FPSCounter
snprintf returns the number of bytes it would have written when it runs out of space (rather than the number of bytes it did write). FPSCounter::PrintHistogram appears to attempt to handle this, but a) the assertion is the opposite of what should be asserted and b) assertions are debug-only (as an aside, if the given histogram is empty, PrintHistogram will just print uninitialized memory). FPSCounter::WriteFrameTimeStamps doesn't properly check the return value either (SprintfLiteral uses vsnprintf, which behaves the same as snprintf in that regard).
I'm not sure if this is exactly the desired behavior, but it's a place to start.
Assignee: nobody → dkeeler
Attachment #8874029 - Flags: review?(mchang)
Version: unspecified → 44 Branch
Comment on attachment 8874029 [details] [diff] [review] patch Review of attachment 8874029 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8874029 - Flags: review?(mchang) → review+
Thanks! I'm marking this as sec-low since it requires changing/adding about:config preferences and since the original implementation is unlikely to overflow the buffers anyway.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4) > Actually it looks like sec-moderate is more appropriate. I disagree since this is a developer debug tool and users shouldn't actually ever expect to use this.
Per comment #3, track 53-/54- as it's unlikely to overflow the buffers.
(To be clear, I'm waiting for bug 1368652 to land before landing this.)
For 55 we have several sec-high and sec-crits that are being tracked. This is a sec-mod that we can uplift to Beta55 if the fix is low-risk. Dropping 55 tracking.
3 years ago
Actually, on second thought, it would be safest to wait until bug 1368652 actually ships (in 55).
bug 1368652 has now shipped, is anything else blocking this one?
I think we're good to go here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8764a29737584ac6902ba5d803f646f175dbcb43
https://hg.mozilla.org/integration/mozilla-inbound/rev/b790f9291cdbad093bb033390f6724cfe8719ac4 Please request Beta approval on this when you get a chance - I've already verified that grafts cleanly. It actually does graft cleanly to ESR52 as well, but doesn't really seem worth the backport there? I'm setting the status to wontfix, but feel free to set it back to affected and request ESR52 approval as well if you feel otherwise.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > b790f9291cdbad093bb033390f6724cfe8719ac4 > > Please request Beta approval on this when you get a chance - I've already > verified that grafts cleanly. It actually does graft cleanly to ESR52 as > well, but doesn't really seem worth the backport there? I'm setting the > status to wontfix, but feel free to set it back to affected and request > ESR52 approval as well if you feel otherwise. I'm still not sure this needs 52 ESR or even is sec-moderate as this is a gecko developer debug flag. Still good to find a security bug, but I don't think this has a high chance of exploit.
Comment on attachment 8874029 [details] [diff] [review] patch Approval Request Comment [Feature/Bug causing the regression]: bug 1197315 [User impact if declined]: it's unlikely non-Firefox-developers would ever be affected by this [Is this code covered by automated tests?]: well, probably not (given that the failing assertion never fired) [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: this is a debug-only feature [String changes made/needed]: none
Attachment #8874029 - Flags: approval-mozilla-beta?
I don't think it's necessary to uplift to ESR52. If nobody's encountered this yet, they're unlikely to.
Comment on attachment 8874029 [details] [diff] [review] patch Fix a security issue. Beta56+.
Attachment #8874029 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.