835290 - WebRTC unit-test global-buffer-overflow crash [@wrap_strlen]

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

This happened while running the unit test: SignalingTest.FullCall

$ media/webrtc/signaling/test/signaling_unittests --gtest_filter=SignalingTest.FullCall

Tested with m-i changeset: 120047:41b5ec7f0293

Comment 1

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

This is the code (in particular the second line):
  description_ = pc_ + "| Receive video[";
  description_ += track_id_ + "]";

The report says the address is "0 bytes to the right of .str843" which is ']'

description_ is a std::string (as is pc_).  I believe TrackID is a uint32_t.

I'm very confused how std::string could cause this.  The code appears totally correct to me.  Bug in std::string???

Comment 2

[Ethan Hugg [:ehugg]]

Another stack from the debugger.  Investigating now.

#0  0x0000000107377111 in __asan_report_error ()
#1  0x0000000107371dbb in wrap_strlen ()
#2  0x00007fff89bc0fe1 in std::string::operator+= ()
#3  0x00000001005219ca in mozilla::MediaPipelineReceiveVideo::Init (this=<value temporarily unavailable, due to optimizations>) at MediaPipeline.cpp:884
#4  0x00000001002f5db1 in vcmRxStartICE_m (mcap_id=<value temporarily unavailable, due to optimizations>, group_id=<value temporarily unavailable, due to optimizations>, stream_id=<value temporarily unavailable, due to optimizations>, level=<value temporarily unavailable, due to optimizations>, pc_stream_id=<value temporarily unavailable, due to optimizations>, pc_track_id=<value temporarily unavailable, due to optimizations>, call_handle=<value temporarily unavailable, due to optimizations>, peerconnection=<value temporarily unavailable, due to optimizations>, num_payloads=<value temporarily unavailable, due to optimizations>, payloads=<value temporarily unavailable, due to optimizations>, fingerprint_alg=<value temporarily unavailable, due to optimizations>, fingerprint=<value temporarily unavailable, due to optimizations>, attrs=<value temporarily unavailable, due to optimizations>) at VcmSIPCCBinding.cpp:1384
#5  0x000000010031601f in mozilla::runnable_args_nm_13_ret<int (*)(unsigned short, unsigned short, unsigned short, int, int, int, unsigned int, char const*, int, vcm_payload_info_t const*, char const*, char const*, vcm_attrs_t_*), unsigned short, unsigned short, unsigned short, int, int, int, unsigned int, char const*, int, vcm_payload_info_t const*, char const*, char const*, vcm_attrs_t_*, int>::Run (this=<value temporarily unavailable, due to optimizations>) at runnable_utils_generated.h:1291
#6  0x000000011c925476 in nsThreadSyncDispatch::Run (this=<value temporarily unavailable, due to optimizations>) at /Users/ehugg/mozilla/mozilla-inbound/xpcom/threads/nsThread.cpp:774
#7  0x000000011c921fcb in nsThread::ProcessNextEvent (this=<value temporarily unavailable, due to optimizations>, mayWait=<value temporarily unavailable, due to optimizations>, result=<value temporarily unavailable, due to optimizations>) at /Users/ehugg/mozilla/mozilla-inbound/xpcom/threads/nsThread.cpp:627
#8  0x000000011c4d7c0b in NS_ProcessNextEvent_P (thread=<value temporarily unavailable, due to optimizations>, mayWait=<value temporarily unavailable, due to optimizations>) at nsThreadUtils.cpp:238
#9  0x000000011c91b4f4 in nsThread::ThreadFunc (arg=<value temporarily unavailable, due to optimizations>) at /Users/ehugg/mozilla/mozilla-inbound/xpcom/threads/nsThread.cpp:265
#10 0x0000000137348443 in _pt_root (arg=<value temporarily unavailable, due to optimizations>) at /Users/ehugg/mozilla/mozilla-inbound/nsprpub/pr/src/pthreads/ptthread.c:156
#11 0x0000000107378a6b in __asan::AsanThread::ThreadStart ()
#12 0x00007fff90e8c181 in thread_start ()

Comment 3

[Ethan Hugg [:ehugg]]

This warning implies we shouldn't be attempting to do += with a std::string and an int

/Users/ehugg/mozilla/mozilla-inbound/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:487:29: warning: adding 'TrackID' (aka 'int') to a string does not append to the string [-Wstring-plus-int]
  description_ += track_id_ + "]";

Comment 4

[Ethan Hugg [:ehugg]]

I will be replacing the std::string += track_id_ which happens in three places in this file with a PR_snprintf.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: MediaPipeline - replace attempted += of ints with PR_snprintfs

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 707242 [details] [diff] [review]
MediaPipeline - replace attempted += of ints with PR_snprintfs

Review of attachment 707242 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ -482,4 @@
>    description_ = pc_ + "| ";
>    description_ += conduit_->type() == MediaSessionConduit::AUDIO ?
>        "Transmit audio[" : "Transmit video[";
> -  description_ += track_id_ + "]";

This does not do what we hoped it would.  Need to use << or C++11's std::to_string, or something like Boost.

@@ +486,5 @@
>    description_ = pc_ + "| ";
>    description_ += conduit_->type() == MediaSessionConduit::AUDIO ?
>        "Transmit audio[" : "Transmit video[";
> +  description_ += track_id_string;
> +  description_ += "]";

Keeping these on the same line will throw an error about no '+' found for char* and const char*

Comment 7

[Ethan Hugg [:ehugg]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c73b57a20d1b

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Can someone suggest a rating for this?

Comment 9

[Ethan Hugg [:ehugg]]

I'

Comment 10

[Ethan Hugg [:ehugg]]

I'm not exactly sure what you mean by rating, but I believe this bug was extremely non-dangerous.  Essentially the += of an int in a std::string treats the int as a char.  If the int happens to be zero which is common in this case, then it truncates the string (description_) and confuses further appending.    I don't think there's any real danger from this bug, but it was fixed so that the descriptions could be correct.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c73b57a20d1b

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Ethan, we rate security bugs using the following guidelines: https://wiki.mozilla.org/Security_Severity_Ratings

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Also, unless a security bug *only* affects trunk (not Aurora, Beta, or shipped versions), it needs to be rated before asking for sec-approval on the patch for checkin. See https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 14

[Al Billings [:abillings - ex-MoCo]]

This is specifically pref'd off in Firefox 20, isn't it?

Comment 15

[Ethan Hugg [:ehugg]]

Yes, the stack calls vcmRxStartICE which would only be called by using an RTCPeerConnection which is pref'd off still in 20.