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)

x86_64
Linux
defect
Not set
normal

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)

Attached file test/valgrind messages (deleted) —
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)
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
Attachment #8390319 - Flags: review?(jib)
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.
Julian, see comment 0 and comment 2
Comment on attachment 8390319 [details] [diff] [review] Initialize mCr/mCb in MediaEngineDefault for fake video color Empty patch.
Attachment #8390319 - Flags: review?(jib) → review-
Attachment #8390319 - Attachment is obsolete: true
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+
Assignee: nobody → rjesup
See glandium's question...
Flags: needinfo?(jseward)
Julian? it's been a month...
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)
> 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.
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)
I don't thing there is anything else actionable here. Let's close this.
Flags: needinfo?(jseward)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: