Closed
Bug 982999
Opened 11 years ago
Closed 10 years ago
Conditional jump or move depends on uninitialised value(s) and Mismatched free() / delete / delete [] with webrtc audio and video
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr31 | --- | wontfix |
People
(Reporter: bc, Assigned: jesup)
References
()
Details
(Keywords: sec-moderate, valgrind)
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
Original log at <http://sisyphus.bughunter.ateam.phx1.mozilla.com/bughunter/media/logs/2014/03/09/12/25/2014-03-09-12-25-21,firefox,nightly,debug,linux,valgrind1.bughunter.ateam.phx1.mozilla.com,crashtest.log>. You'll need MozillaVPN with the right acls.
This is from a build on 2014/03/09 so the line numbers might not match with current trunk. I can't run valgrind locally since it will interfere with another test framework I am running. Let me know if that is a problem.
dom/media/tests/crashtests/855796.html
dom/media/tests/crashtests/799419.html
dom/media/tests/crashtests/863929.html
crashtests chunk 10 of 40
rm -f ./crashtest.log && /work/mozilla/builds/nightly/mozilla/firefox-debug/_virtualenv/bin/python _tests/reftest/runreftest.py --extra-profile-file=dist/plugins --symbols-path=dist/crashreporter-symbols --debugger='valgrind' --debugger-args='--tool=memcheck --vex-iropt-register-updates=allregs-at-each-insn --smc-check=all-non-file --soname-synonyms=somalloc=NONE --trace-children=yes --track-origins=yes --read-var-info=yes' --timeout=86400 --total-chunks=40 --this-chunk=10 '/work/mozilla/builds/nightly/mozilla/testing/crashtest/crashtests.list' | tee ./crashtest.log
The allocation site for the Uninitialized value allocation is the same:
Uninitialised value was created by a heap allocation
at 0x4A05AF3: malloc (vg_replace_malloc.c:293)
by 0x63B27DB: moz_xmalloc (mozalloc.cpp:52)
by 0x84E8450: mozilla::MediaEngineDefault::EnumerateVideoDevices(nsTArray >*) (mozalloc.h:201)
by 0x7FA84EC: nsTArray >* mozilla::GetSources(mozilla::MediaEngine*, mozilla::dom::MediaTrackConstraintsInternal const&, void (mozilla::MediaEngine::*)(nsTArray >*), char*) (MediaManager.cpp:685)
by 0x7FA8A1F: mozilla::GetUserMediaRunnable::SelectDevice(mozilla::MediaEngine*) (MediaManager.cpp:940)
by 0x7FA8DB2: mozilla::GetUserMediaRunnable::Run() (MediaManager.cpp:851)
by 0x6EE3C63: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:643)
by 0x6E5F450: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:263)
by 0x71E3E81: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (MessagePump.cpp:332)
by 0x71C8137: MessageLoop::RunInternal() (message_loop.cc:226)
by 0x71C8164: MessageLoop::Run() (message_loop.cc:219)
by 0x6EE61C4: nsThread::ThreadFunc(void*) (nsThread.cpp:258)
The allocation sites for the Mismatched free() / delete / delete [] differ but all involve
by 0x7498AEE: mozilla::WebrtcAudioConduit::Init(mozilla::WebrtcAudioConduit*) (AudioConduit.cpp:319)
by 0x749A0DD: mozilla::AudioSessionConduit::Create(mozilla::AudioSessionConduit*) (AudioConduit.cpp:51)
or
by 0x749CA17: mozilla::WebrtcVideoConduit::Init(mozilla::WebrtcVideoConduit*) (VideoConduit.cpp:318)
by 0x749CE72: mozilla::VideoSessionConduit::Create(mozilla::VideoSessionConduit*) (VideoConduit.cpp:43)
Assignee | ||
Comment 1•11 years ago
|
||
this is totally safe even if unitialized; this is just the color for the fake video, and it uses the current value to decide the next one
Assignee | ||
Updated•11 years ago
|
Attachment #8390319 -
Flags: review?(jib)
Comment 2•11 years ago
|
||
The free/delete/delete[] mismatches might be due to different inlining of operator new/delete. In general, those don't matter because everything goes through malloc/free anyways in our builds. I think we gave up on these mismatches. Sewardj probably knows more.
Comment 3•11 years ago
|
||
Comment 4•11 years ago
|
||
Comment on attachment 8390319 [details] [diff] [review]
Initialize mCr/mCb in MediaEngineDefault for fake video color
Empty patch.
Attachment #8390319 -
Flags: review?(jib) → review-
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8390319 -
Attachment is obsolete: true
Comment 6•11 years ago
|
||
Comment on attachment 8390333 [details] [diff] [review]
Initialize mCr/mCb in MediaEngineDefault for fake video color
Review of attachment 8390333 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm.
::: content/media/webrtc/MediaEngineDefault.cpp
@@ +34,5 @@
> * Default video source.
> */
>
> MediaEngineDefaultVideoSource::MediaEngineDefaultVideoSource()
> +: mTimer(nullptr), mMonitor("Fake video"), mCb(0), mCr(0)
Couldn't hurt to init mTrackID as well, though it appears safe.
Attachment #8390333 -
Flags: review+
Updated•11 years ago
|
Assignee: nobody → rjesup
Assignee | ||
Comment 8•11 years ago
|
||
Julian? it's been a month...
Comment 9•11 years ago
|
||
I can't reproduce the free/delete/delete[] using a jemalloc build, either at
-O or -O2. On looking at the logs from Bob, I suspect that they are caused
by differential inlining: gcc 4.5 has inlined more on the new side than the
delete side (or the other way around).
But with gcc-4.7.2 on Fedora 17, I can't reproduce this.
I *do* still see the uninit value errors. So I'd suggest to fix those
and forget about the mismatches.
Flags: needinfo?(jseward)
Comment 10•11 years ago
|
||
> So I'd suggest to fix those and forget about the mismatches.
Having said that, the presence of these mismatch reports is
still a hindance to automated testing, and we need to get rid of
them somehow.
Assignee | ||
Comment 11•10 years ago
|
||
the patch for the color values landed long ago (in a different patch/bug). Is there more actionable here? If not, let's close it
Flags: needinfo?(jseward)
Comment 12•10 years ago
|
||
I don't thing there is anything else actionable here. Let's close this.
Flags: needinfo?(jseward)
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
status-firefox-esr31:
--- → wontfix
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•