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)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

Details

Attachments

(2 files)

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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: