Closed
Bug 1319623
Opened 8 years ago
Closed 8 years ago
Valgrind reports uninitialized memory use in pulse_stream_set_panning running cubeb.run_panning_volume_test_short gtest
Categories
(Core :: Audio/Video: cubeb, defect, P1)
Core
Audio/Video: cubeb
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: kinetik, Assigned: kinetik)
Details
Attachments
(2 files)
32.08 KB,
text/plain
|
Details | |
841 bytes,
patch
|
Details | Diff | Splinter Review |
Julian pinged me on IRC with this Valgrind warning: https://pastebin.mozilla.org/8930635
[ RUN ] cubeb.run_panning_volume_test_short
[...]
Panning: -1.00%
==24706== Conditional jump or move depends on uninitialised value(s)
==24706== at 0x1831E33A: pa_cvolume_valid (/usr/src/debug/pulseaudio-7.1/src/pulse/volume.c:533)
==24706== by 0x1831F9AC: pa_cvolume_compatible_with_channel_map (/usr/src/debug/pulseaudio-7.1/src/pulse/volume.c:632)
==24706== by 0x183203F0: pa_cvolume_set_balance (/usr/src/debug/pulseaudio-7.1/src/pulse/volume.c:708)
==24706== by 0x154637EA: pulse_stream_set_panning (MC-TALL/media/libcubeb/src/cubeb_pulse.c:1018)
==24706== by 0x159FD100: run_panning_volume_test(int) (MC-TALL/media/libcubeb/gtest/test_audio.cpp:235)
Where pulse_stream_set_panning passes an uninitialized pa_cvolume to pa_cvolume_set_balance.
Looking at the code in question, it turns out it doesn't *do* anything with the pa_cvolume it's trying to prepare. It's a toss up between removing the existing partial implementation and returning CUBEB_ERROR_NOT_SUPPORTED (since we only rely on this API on macOS right now, AFAIK) or completing them implementation.
Assignee | ||
Comment 1•8 years ago
|
||
This will be fixed by completing the API implementation in https://github.com/kinetiknz/cubeb/pull/201
Comment 2•8 years ago
|
||
V results for all the cubeb tests.
Running gtests on Valgrind is a bit tricky, but the following works
(modulo fiddling with objdir paths):
DISPLAY=:1.0 G_SLICE=always-malloc MOZILLA_FIVE_HOME= \
MOZ_XRE_DIR=/home/sewardj/MOZ/MC-TALL/ff-O2-linux64/dist/bin \
MOZ_GMP_PATH=/home/sewardj/MOZ/MC-TALL/ff-O2-linux64/dist/bin/gmp-fake/1.0:/home/sewardj/MOZ/MC-TALL/ff-O2-linux64/dist/bin/gmp-fakeopenh264/1.0 \
XPCOM_DEBUG_BREAK=stack-and-abort MOZ_CRASHREPORTER_NO_REPORT=1 \
MOZ_CRASHREPORTER=1 MOZ_RUN_GTEST=1 \
GTEST_FILTER="cubeb*" \
\
/home/sewardj/VgTRUNK/progress/Inst/bin/valgrind --fair-sched=yes \
--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc-may2015.supp \
--error-limit=no --trace-children=yes --child-silent-after-fork=yes \
--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java \
--num-transtab-sectors=24 --tool=memcheck --freelist-vol=300000000 \
--redzone-size=64 --gen-suppressions=no --px-default=allregs-at-mem-access \
--px-file-backed=unwindregs-at-mem-access --stats=no \
--show-mismatched-frees=no --fullpath-after=/MOZ/ --num-callers=30 \
--track-origins=yes \
/home/sewardj/MOZ/MC-TALL/ff-O2-linux64/dist/bin/firefox -unittest \
\
2>&1 | tee spew-14-29-mc-gtest
Comment 3•8 years ago
|
||
Trivial "fix": memset-zero the |vol| struct.
Comment 4•8 years ago
|
||
(In reply to Matthew Gregan [:kinetik] from comment #1)
> This will be fixed by completing the API implementation in
> https://github.com/kinetiknz/cubeb/pull/201
What kind of timescale would this involve? Is it worth landing
the band-aid in comment 3 in the meantime?
Updated•8 years ago
|
Rank: 15
Priority: -- → P1
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Julian Seward [:jseward] from comment #4)
> (In reply to Matthew Gregan [:kinetik] from comment #1)
> > This will be fixed by completing the API implementation in
> > https://github.com/kinetiknz/cubeb/pull/201
>
> What kind of timescale would this involve? Is it worth landing
> the band-aid in comment 3 in the meantime?
I'll land the fix on inbound today.
If we need something quicker, your memset works, but it's better to delete the body of the function and replace it with a CUBEB_ERROR_NOT_SUPPORTED return, since it does nothing useful as-is anyway.
Pushed by mgregan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d48f8dd5d4f
Update libcubeb to 051cd847. r=achronop
Comment 7•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•