[MIR] webp-pixbuf-loader

Review for Package: webp-pixbuf-loader

[Summary]
MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.

This does need a security review, so I'll assign ubuntu-security

List of specific binary packages to be promoted to main: webp-pixbuf-loader
Specific binary packages built, but NOT to be promoted to main: -

Notes:
Required TODOs:
- Automated tests do not seem too complex to add, please:
  - bump the testsuite to also run at autopkgtest time
  - consider adding a set of "known content" webp pictures that you can
    compare against expected results in the way the gnome thumbnailer will
    use them (details see below).

Recommended TODOs:
- fix the s390x build before kinetic releases

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - libc6 (>= 2.14), libgdk-pixbuf-2.0-0 (>= 2.38.1), libglib2.0-0 (>= 2.37.3),
    libwebp7, libwebpdemux2 are all in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring
  more tests now.

Problems: None

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have odd Built-Using entries
- not a go package, no extra constraints to consider in that regard

Problems: None

[Security]
OK:
- history of CVEs does not look concerning
  - but webp in general (as all media formats) had a bunch
    https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=webp
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)
- does not deal with cryptography (en-/decryption, certificates, signing, ...)

Problems:
- does not parse data formats (files [images, video, audio,
  xml, json, asn.1], network packets, structures, ...) from
  an untrusted source.
  To be fair There isn't much code and the actual content handling is
  primarily in dependencies libwebp7 and libgdk-pixbuf-2.0-0.
  But such code has been attack surface often enough, it should be checked.

[Common blockers]
OK:
- does have a test suite that runs at build time
  - test suite fails will fail the build upon error.
- No special HW needed
- no new python2 dependency

Problems:
- does FTBFS currently (on s390x)
  You have filed this upstream, sicne s390x has no GUI this isn't a blocker
  but would be great to be resolved before kinetic releases
- does not have a non-trivial test suite that runs as autopkgtest.
  You've said you might want to do that as manual test before upload, but
  TBH we all know they are often too easily forgotten and have no way to catch
  dependencies breaking you.
  Furthermore in this case it doesn't even seem too hard to add tests.
  1. The runtime tests are good - please run that in autopkgtest context will
     easily allow to catch dependencies changing in a bad way
  2. It seems deterministic, so yo...

The attached patch implements the required autopkgtests.

They were tested with autopkgtest + schroot.

Thanks Nathan, nice work. I think the standard way of building the source for the test is to use in debian/tests/control

Restrictions: build-needed

(documented in https://salsa.debian.org/ci-team/autopkgtest/raw/master/doc/README.package-tests.rst)

then 'cd obj-*; meson test'

umockdev is doing that as an example

the cmp test also should be enough, if we have exact matching to point trying to do a fuzzy one.

could you tweak the upstream one to use build-needed and I will do the sponsoring

Thank you for the review, Sebastien.

For reference, after discussion we decided to

- skip build-needed.

- modify the way upstream tests are triggered so that it doesn't use the rebuilt source tree for tests. For additional context: https://irclogs.ubuntu.com/2022/06/28/%23ubuntu-devel.txt.

@Christian, we uploaded https://launchpad.net/ubuntu/+source/webp-pixbuf-loader/0.0.5-2ubuntu1 now which fixes the recommended TODO

It includes a cherry pick from an upstream fix for the big endian issue, the update built on s390x now

The update includes the autopkgtest that Nathan worked on following your suggestion, one is running the upstream testsuite and the other one converting to a png and comparing the file to the expected output

This bug was fixed in the package webp-pixbuf-loader - 0.0.5-2ubuntu1

---------------
webp-pixbuf-loader (0.0.5-2ubuntu1) kinetic; urgency=medium

  * debian/patches/git_big_endian.patch:
    - cherry pick an upstream fix for big endian architectures

  [ Nathan Pratta Teodosio ]
  * Add autopkgtests to debian/tests (lp: #1979121)

 -- Sebastien Bacher <email address hidden> Tue, 28 Jun 2022 21:43:57 +0200

This bug was fixed in the package webp-pixbuf-loader - 0.0.5-5

---------------
webp-pixbuf-loader (0.0.5-5) unstable; urgency=medium

  * Add build-needed to the autopkgtest spec.

 -- Andrej Shadura <email address hidden> Wed, 29 Jun 2022 11:31:28 +0200

is there plan to integrate it into gnome metapackage (or any other)?

I reviewed webp-pixbuf-loader 0.0.5-5 as checked into kinetic. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

webp-pixbuf-loader is a loadable gdk-pixbuf module that contains the necessary
functions to load and save images in webp format.

- CVE History:
  - No history of CVEs.
- Build-Depends?
  - No direct dependency on encryption or networking libraries.
- pre/post inst/rm scripts?
  - No.
- init scripts?
  - No.
- systemd units?
  - No.
- dbus services?
  - No.
- setuid binaries?
  - No.
- binaries in PATH?
  - No.
- sudo fragments?
  - No.
- polkit files?
  - No.
- udev rules?
  - No.
- unit tests / autopkgtests?
  - Unit tests and some trivial determinism autopkgtests included and successfully run.
- cron jobs?
  - No.
- Build logs:
  - A couple of deprecation warnings in the test files like:
    [4/16] cc -Itests/t3.p -Itests -I../tests -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng16 -I/usr/include/x86_64-linux-gnu -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O0 -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -flto=auto -ffat-lto-objects -flto=auto -ffat-lto-objects -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -pthread -MD -MQ tests/t3.p/t3.c.o -MF tests/t3.p/t3.c.o.d -o tests/t3.p/t3.c.o -c ../tests/t3.c
    ../tests/t3.c: In function ‘main’:
    ../tests/t3.c:17:17: warning: ‘GTimeVal’ is deprecated: Use 'GDateTime' instead [-Wdeprecated-declarations]
       17 | GTimeVal curTime;
          | ^~~~~~~~
  - No Lintian failures.
- Processes spawned?
  - No.
- Memory management?
  - Uses glib wrappers for memory management. Several resource leaks as reported by coverity and also verified with code review.
  - io-webp.c:173 core_used_len + size could wrap although impossible for size_t even for 32-bit systems.
- File IO?
  - io-webp-anim.c:467 get_data_from_file receives a pointer to FILE and reads raw data from it. No filepath handling.
  - io-webp.c:48 gdk_pixbuf__webp_image_load receives a pointer to FILE and reads raw data from it. No filepath handling.
- Logging?
  - Uses mostly g_set_error from glib. No format strings or other issues found.
- Environment variable usage?
  - No.
- Use of privileged functions?
  - No.
- Use of cryptography / random number sources etc?
  - No.
- Use of temp files?
  - No.
- Use of networking?
  - No.
- Use of WebKit?
  - No
- Use of PolicyKit?
  - No
- Any significant cppcheck results?
  - No.
- Any significant Coverity results?
  - SEE coverity.txt. A couple of resource leaks were reported as well as missing check when using ftell's return value (which can be -1) as a malloc argument.
- Any significant shellcheck results?
  - No

Security team ACK for promoting webp-pixbuf-loader to main.

After this week MIR meeting, +1 from us.

Override component to main
webp-pixbuf-loader 0.0.5-5 in kinetic: universe/misc -> main
webp-pixbuf-loader 0.0.5-5 in kinetic amd64: universe/gnome/optional/100% -> main
webp-pixbuf-loader 0.0.5-5 in kinetic arm64: universe/gnome/optional/100% -> main
webp-pixbuf-loader 0.0.5-5 in kinetic armhf: universe/gnome/optional/100% -> main
webp-pixbuf-loader 0.0.5-5 in kinetic ppc64el: universe/gnome/optional/100% -> main
webp-pixbuf-loader 0.0.5-5 in kinetic riscv64: universe/gnome/optional/100% -> main
webp-pixbuf-loader 0.0.5-5 in kinetic s390x: universe/gnome/optional/100% -> main
Override [y|N]? y
7 publications overridden.

Targetting 22.04 there now as we would like to have webp support enabled by default also in the LTS.

@MIR team, could you review the request of promoting the package also in 22.04? The exact same version that was promoted in Kinetic has been SRUed to the LTS, https://launchpad.net/bugs/1993789

If/when the promotion is validated we will work on SRUing an eog update pulling webp-pixbuf-loader by default (depends or recommends)

Sorry Seb that this has fallen through the cracks :-/

I've seen and confirmed that this is indeed the same that was reviewed and approved in Kinetic (0.0.5-5~22.04.1).

So all former statements apply and this is IMHO indeed good to go.

It does not add huge extra load for maintenance as going forward it will be in LTSes anyway.
Recently this got way more mature, at least in terms of version number 0.0.5 -> 0.2.4 sounds like a lot.
But since 0.0.5 was reviewed and approved by MIR and security team that does not need to stop us here.

The tests that you added look good on jammy:
- https://autopkgtest.ubuntu.com/packages/w/webp-pixbuf-loader/jammy/amd64

Approving for promoting this in Jammy as well.

Am I missing something? Wouldn't this be solved by merely adding webp-pixbuf-loader as a dependency? Adding it solves the missing file upload previews, for me, in both Kubuntu & Ultramarine Plasma (Fedora).

I'm going to close the 22.04 target there, it was an improvement we wanted to land at the time of the request but the review delayed us and priorities shifted. Now we have a newer LTS out and enough SRU backlog; we will prioritize other changes.