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

Status: RESOLVED FIXED

Description

[Julian Seward [:jseward]]

[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);

Comment 1

[Julian Seward [:jseward]]

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      ^~~~~~

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix memset sizes in rtt_filter.cc

MozReview-Commit-ID: 3pnTseY4k2M

Comment 4

[johannkoenig]

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.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4404f99d719a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4404f99d719a

Please nominate this for Aurora/Beta when you get a chance.

Comment 7

[Julian Seward [:jseward]]

Randell, is the comment 1 anomaly also something we should fix
in this bug, or shall I file a new one?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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)

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Julien Cristau [:jcristau] (back on August 5)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d4a9c3ed6ed9
https://hg.mozilla.org/releases/mozilla-beta/rev/c4d7b21349ea