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

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Matthew Gregan [:kinetik]]

This will be fixed by completing the API implementation in https://github.com/kinetiknz/cubeb/pull/201

Comment 2

[Julian Seward [:jseward]]

Attachment: V results for all the cubeb tests

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

[Julian Seward [:jseward]]

Attachment: bug1319623-1.cset

Trivial "fix": memset-zero the |vol| struct.

Comment 4

[Julian Seward [:jseward]]

(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?

Comment 5

[Matthew Gregan [:kinetik]]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d48f8dd5d4f
Update libcubeb to 051cd847.  r=achronop

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d48f8dd5d4f

Comment 8

[Intermittent Failures Robot]

3 failures in 2673 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 3

Platform breakdown:

• windows10-aarch64: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1319623&startday=2019-04-15&endday=2019-04-21&tree=all