Closed
Bug 1333752
Opened 6 years ago
Closed 6 years ago
Incorrect size for memset in VCMRttFilter::Reset() and libvpx
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: jseward, Assigned: jesup)
References
Details
(Keywords: csectype-uninitialized, sec-low, Whiteboard: [post-critsmash-triage][adv-main52+])
Attachments
(1 file)
1.12 KB,
patch
|
dminor
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
[Please declassify as appropriate] In media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc void VCMRttFilter::Reset() we have memset(_jumpBuf, 0, kMaxDriftJumpCount); memset(_driftBuf, 0, kMaxDriftJumpCount); where enum { kMaxDriftJumpCount = 5 }; but unfortunately int64_t _jumpBuf[kMaxDriftJumpCount]; int64_t _driftBuf[kMaxDriftJumpCount]; The effect is to zero out only the first 5 bytes rather than (as was presumably intended) the whole 40. ------ The comment preceding |enum { kMaxDriftJumpCount = 5 };| is somewhat vague: // The size of the drift and jump memory buffers // and thus also the detection threshold for these // detectors in number of samples. enum { kMaxDriftJumpCount = 5 }; Size in what units? It should really say. ------ This was found when building Gecko with a gcc 7 prerelease, with -O2 and -Wall. 25:51.67 /home/sewardj/MOZ/MC-TALL/media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc: In member function ‘void webrtc::VCMRttFilter::Reset()’: 25:51.67 Warning: -Wmemset-elt-size in /home/sewardj/MOZ/MC-TALL/media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc: ‘memset’ used with length equal to number of elements without multiplication by element size 25:51.67 /home/sewardj/MOZ/MC-TALL/media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc:52:41: warning: ‘memset’ used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size] 25:51.67 memset(_jumpBuf, 0, kMaxDriftJumpCount); 25:51.67 ^ 25:51.67 Warning: -Wmemset-elt-size in /home/sewardj/MOZ/MC-TALL/media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc: ‘memset’ used with length equal to number of elements without multiplication by element size 25:51.67 /home/sewardj/MOZ/MC-TALL/media/webrtc/trunk/webrtc/modules/video_coding/rtt_filter.cc:53:42: warning: ‘memset’ used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size] 25:51.67 memset(_driftBuf, 0, kMaxDriftJumpCount);
Reporter | ||
Comment 1•6 years ago
|
||
There is a similar anomaly reported for media/libvpx/libvpx/vp8/decoder/onyxd_if.c, vp8_create_decoder_instances(): memset(fb->pbi, 0, sizeof(fb->pbi) / sizeof(fb->pbi[0])); where struct VP8D_COMP *pbi[MAX_FB_MT_DEC]; This appears to zero out the first MAX_FB_MT_DEC bytes of the array rather than the first MAX_FB_MT_DEC elements. 35:41.23 /home/sewardj/MOZ/MC-TALL/media/libvpx/libvpx/vp8/decoder/onyxd_if.c: In function ‘vp8_create_decoder_instances’: 35:41.23 Warning: -Wmemset-elt-size in /home/sewardj/MOZ/MC-TALL/media/libvpx/libvpx/vp8/decoder/onyxd_if.c: ‘memset’ used with length equal to number of elements without multiplication by element size 35:41.23 /home/sewardj/MOZ/MC-TALL/media/libvpx/libvpx/vp8/decoder/onyxd_if.c:449:5: warning: ‘memset’ used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size] 35:41.23 memset(fb->pbi, 0, sizeof(fb->pbi) / sizeof(fb->pbi[0])); 35:41.23 ^~~~~~
Assignee | ||
Comment 2•6 years ago
|
||
It's unclear without more analysis if there's even a chance of uninited memory being accessed here (and ASAN doesn't trip on it that we've seen, so I suspect there isn't). Even if there is, the only issue would be confusion over the RTT value, so little if any security/privacy impact. Probably should unmark. Note: rtt_filter.cc moved in 53, and got coding-style mods, but didn't change in any relevant way.
Assignee: nobody → rjesup
Rank: 19
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → wontfix
Keywords: csectype-uninitialized,
sec-low
Priority: -- → P1
Summary: Incorrect use of memset in VCMRttFilter::Reset() → Incorrect size for memset in VCMRttFilter::Reset() and libvpx
Assignee | ||
Comment 3•6 years ago
|
||
MozReview-Commit-ID: 3pnTseY4k2M
Attachment #8830302 -
Flags: review?(dminor)
Comment 4•6 years ago
|
||
I've opened an issue upstream: https://bugs.chromium.org/p/webm/issues/detail?id=1364 This doesn't look like a security issue for libvpx.
Updated•6 years ago
|
Attachment #8830302 -
Flags: review?(dminor) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4404f99d719a
Comment 6•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4404f99d719a Please nominate this for Aurora/Beta when you get a chance.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(rjesup)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Comment 7•6 years ago
|
||
Randell, is the comment 1 anomaly also something we should fix in this bug, or shall I file a new one?
Assignee | ||
Comment 8•6 years ago
|
||
Open a separate bug for clarity (different upstream project's code); see comment 4 for more details on this libvpx issue (johann is a libvpx maintainer who landed the 1.6.1 update last week)
Flags: needinfo?(rjesup)
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8830302 [details] [diff] [review] Fix memset sizes in rtt_filter.cc Approval Request Comment [Feature/Bug causing the regression]: N/A (upstream import; no recent change) [User impact if declined]: Probably nothing; ASAN hasn't triggered on this running calls on Test machines or locally. Worst case would be inaccurate RTT tracking. [Is this code covered by automated tests?]: yes indirectly [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: No way to test [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: Not at all [Why is the change risky/not risky?]: obvious fix for the memset to use sizeof() [String changes made/needed]: none
Attachment #8830302 -
Flags: approval-mozilla-beta?
Attachment #8830302 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 10•6 years ago
|
||
Comment on attachment 8830302 [details] [diff] [review] Fix memset sizes in rtt_filter.cc fix memset usage, aurora53+, beta52+ Per your comment 2 in beta this applies to media/webrtc/trunk/webrtc/modules/video_coding/main/source/rtt_filter.cc instead.
Attachment #8830302 -
Flags: approval-mozilla-beta?
Attachment #8830302 -
Flags: approval-mozilla-beta+
Attachment #8830302 -
Flags: approval-mozilla-aurora?
Attachment #8830302 -
Flags: approval-mozilla-aurora+
Comment 11•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d4a9c3ed6ed9 https://hg.mozilla.org/releases/mozilla-beta/rev/c4d7b21349ea
Updated•6 years ago
|
Group: core-security → core-security-release
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•