Valgrind reports uninitialized memory use in pulse_stream_set_panning running cubeb.run_panning_volume_test_short gtest

RESOLVED FIXED in Firefox 53

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

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.
This will be fixed by completing the API implementation in https://github.com/kinetiknz/cubeb/pull/201
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
Trivial "fix": memset-zero the |vol| struct.
(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?
Rank: 15
Priority: -- → P1
(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
https://hg.mozilla.org/mozilla-central/rev/4d48f8dd5d4f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.