Comment 26 for bug 1802533

Revision history for this message
Joy Latten (j-latten) wrote :

This second review will only document the areas that some difference was found from the first review.

I reviewed pipewire 0.3.15-1 as checked into hirsute. This shouldn't be considered a full audit but rather a quick gauge of maintainability.

- Build-Depends:
debhelper-compat (= 13), libasound2-dev, libbluetooth-dev, libdbus-1-dev, libglib2.0-dev (>= 2.32.0), libgstreamer-plugins-base1.0-dev, libgstreamer1.0-dev, libjack-jackd2-dev (>= 1.9.10), libpulse-dev (>= 11.1), libsbc-dev, libsdl2-dev, libsndfile1-dev (>= 1.0.20), libsystemd-dev,
libudev-dev, libv4l-dev, meson (>= 0.50.0), pkg-config (>= 0.22), systemd, xmltoman, doxygen, graphviz

- pre/post inst/rm scripts:
dh_installsystemduser automatically adds postinst scripts to enable the pipewire.service and pipewire.socket units.
dh_installsystemduser automatically adds a postrm that removes or purges the pipewire.socket and pipewire.service.

- udev rules : 90-pipewire-alsa.rules

- autopkgtests - 3 bash scripts to test interaction with gnome,gstreamer and libpipewire. There are also tests integrated into the source code. They are run during the build cycle. There are also examples and tests packaged in pipewire.test pkg.

- Build logs: Built successfully. However, because the code contains unusual characters in comments, there were many "bogus" warnings during the build. i.e.,
/** \class pw_filter
 *
 * \brief PipeWire filter object class
 *
 * The filter object provides a convenient way to implement
 * processing filters.
 *
 * See also \ref page_filters and \ref page_core_api
 */

- LINTIAN ran successfully with some errors and warnings:
E: pipewire changes: bad-distribution-in-changes-file unstable
E: pipewire-audio-client-libraries: custom-library-search-path usr/lib/x86_64-linux-gnu/pipewire-0.3/pulse/libpulse-mainloop-glib.so.0.315.0 /usr/${LIB}/pipewire-0.3/pulse
E: pipewire-audio-client-libraries: custom-library-search-path usr/lib/x86_64-linux-gnu/pipewire-0.3/pulse/libpulse-simple.so.0.315.0 /usr/${LIB}/pipewire-0.3/pulse
E: pipewire-audio-client-libraries: library-not-linked-against-libc usr/lib/x86_64-linux-gnu/pipewire-0.3/jack/libjacknet.so.0.315.0
E: pipewire-audio-client-libraries: library-not-linked-against-libc usr/lib/x86_64-linux-gnu/pipewire-0.3/jack/libjackserver.so.0.315.0
W: pipewire-bin: no-manual-page usr/bin/pipewire-media-session
W: pipewire-bin: no-manual-page usr/bin/pw-reserve
W: pipewire-bin: no-manual-page usr/bin/spa-acp-tool
W: pipewire-bin: no-manual-page usr/bin/spa-inspect
W: pipewire-bin: no-manual-page usr/bin/spa-monitor
W: pipewire-bin: no-manual-page usr/bin/spa-resample
N: 7 tags overridden (7 errors)

- spawns a daemon, code looks ok.

- Memory management:
Quite a bit of malloc|calloc|realloc used without checking return value before use. Especially in spa/plugins.

- A lot of environment variables. Looking at a random sampling, code-wise looks ok, but use of
them in some places may be questionable. i.e.
1. - pw-pulse.in and pw-jack.in shell scripts use and modify LD_LIBRARY_PATH so applications load pipewire's pulseaudio or jack instead of Jack's and PulseAudio's.
2. The pipewire daemon uses env vars to set alternative name and config ile for the daemon. The name can also be set with a cmdline option to the daemon. So can change the name in 2 different places.
3. pw_init() contains env vars when the daemon initializes to change defaults such as the spa plugin directory. Wonder why not use a config file for some of these?

- cppcheck reports a lot of uninitialized variables.

Conclusions:
Significant source code growth and changes since first security MIR review.
Code base seems to be transitioning from new development to stability.
Security team ACK for promoting pipewire to main.