Incorrect size for memset in VCMRttFilter::Reset() and libvpx

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
Rank:
19
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: jseward, Assigned: jesup)

Tracking

({csectype-uninitialized, sec-low})

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 attachment)

[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);
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      ^~~~~~
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
Priority: -- → P1
Summary: Incorrect use of memset in VCMRttFilter::Reset() → Incorrect size for memset in VCMRttFilter::Reset() and libvpx
MozReview-Commit-ID: 3pnTseY4k2M
Attachment #8830302 - Flags: review?(dminor)
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.
Attachment #8830302 - Flags: review?(dminor) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/4404f99d719a

Please nominate this for Aurora/Beta when you get a chance.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(rjesup)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Randell, is the comment 1 anomaly also something we should fix
in this bug, or shall I file a new one?
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)
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?
Keywords: leave-open
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+
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
See Also: → 1480374
You need to log in before you can comment on or make changes to this bug.