Comment 4 for bug 2004523

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote : Re: [MIR] libwebm (transitive dependency of libheif)

Review for Package: libwebm

[Summary]
MIR team ack under the constraints 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.

Also, all dependencies needs to be MIR acked.

Required TODOs:
- It’s not available on s390x due to the test failing on that architecture. Given that we have time to investigate and there is no rationale to skip that arch, which is officially supported, I think we should fix it first before promoting it to main.
- does not have an autopkgtest suite at all. This is a MIR requirement that can’t be skipped or it needs a manual test plan being run as every release. Maybe you can take some of the tests running at build time if they are testing your package against other third parties?
Recommended TODOs:
- The package should get a team bug subscriber before being promoted
- some warnings during the build. I think those could be fixed or marked as false positive:
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:135:5: warning: ‘t.milliseconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc:120:19: note: ‘t.milliseconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator<’ at ./webvtt/webvttparser.cc:626:6,
    inlined from ‘ParseChapters’ at ./sample_muxer_metadata.cc:135:22:
./webvtt/webvttparser.cc:642:3: warning: ‘t.seconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:120:19: note: ‘t.seconds’ was declared here
In member function ‘operator<’,
    inlined from ‘ParseChapters’ at ./sample_muxer_metadata.cc:135:22:
./webvtt/webvttparser.cc:636:3: warning: ‘t.minutes’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:120:19: note: ‘t.minutes’ was declared here
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:289:5: warning: ‘t.milliseconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc:274:19: note: ‘t.milliseconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator<’ at ./webvtt/webvttparser.cc:626:6,
    inlined from ‘operator>=’ at ./webvtt/webvttparser.cc:652:71,
    inlined from ‘Parse’ at ./sample_muxer_metadata.cc:289:22:
./webvtt/webvttparser.cc:642:3: warning: ‘t.seconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:274:19: note: ‘t.seconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator>=’ at ./webvtt/webvttparser.cc:652:71,
    inlined from ‘Parse’ at ./sample_muxer_metadata.cc:289:22:
./webvtt/webvttparser.cc:636:3: warning: ‘t.minutes’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:274:19: note: ‘t.minutes’ was declared here

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

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - aom checked with `check-mir`
  - all dependencies can be found in `seeded-in-ubuntu` (already in main)
 - none of the (potentially auto-generated) dependencies (Depends
   and Recommends) that are present after build are not in main
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring more tests now.

[Embedded sources and static linking]
OK:
TODO: - no embedded source present
TODO: - no static linking
TODO: - does not have unexpected Built-Using entries

[Security]
OK:
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- 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:
- quite some history of CVEs
- does parse data formats (files [images, video, audio, xml, json, asn.1], network packets, structures, ...) from an untrusted source.

Those are the reasons for needing a security review.

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

Problems:
- It’s not available on s390x due to the test failing on that architecture. Given that we have time to investigate and there is no rationale to skip that arch, which is officially supported, I think we should fix it first before promoting it to main.
- does not have an autopkgtest suite at all. This is a MIR requirement that can’t be skipped or it needs a manual test plan being run as every release. Maybe you can take some of the tests running at build time if they are testing your package against other third parties?

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking is in place
- d/watch is present and looks ok (if needed, e.g. non-native)
- Upstream update history is good
- Debian/Ubuntu update history is good
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- It is not on the lto-disabled list

[Upstream red flags]
OK:
- no incautious use of malloc/sprintf (as far as we can check it)
- no incautious use of malloc/sprintf (the language has no direct MM)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside tests)
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- not part of the UI for extra checks
- no translation present, but none needed for this case.

Problems:
- some warnings during the build. I think those could be fixed or marked as false positive:
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:135:5: warning: ‘t.milliseconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc:120:19: note: ‘t.milliseconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator<’ at ./webvtt/webvttparser.cc:626:6,
    inlined from ‘ParseChapters’ at ./sample_muxer_metadata.cc:135:22:
./webvtt/webvttparser.cc:642:3: warning: ‘t.seconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:120:19: note: ‘t.seconds’ was declared here
In member function ‘operator<’,
    inlined from ‘ParseChapters’ at ./sample_muxer_metadata.cc:135:22:
./webvtt/webvttparser.cc:636:3: warning: ‘t.minutes’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In function ‘ParseChapters’:
./sample_muxer_metadata.cc:120:19: note: ‘t.minutes’ was declared here
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:289:5: warning: ‘t.milliseconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc:274:19: note: ‘t.milliseconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator<’ at ./webvtt/webvttparser.cc:626:6,
    inlined from ‘operator>=’ at ./webvtt/webvttparser.cc:652:71,
    inlined from ‘Parse’ at ./sample_muxer_metadata.cc:289:22:
./webvtt/webvttparser.cc:642:3: warning: ‘t.seconds’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:274:19: note: ‘t.seconds’ was declared here
In member function ‘operator<’,
    inlined from ‘operator>=’ at ./webvtt/webvttparser.cc:652:71,
    inlined from ‘Parse’ at ./sample_muxer_metadata.cc:289:22:
./webvtt/webvttparser.cc:636:3: warning: ‘t.minutes’ may be used uninitialized [-Wmaybe-uninitialized]
./sample_muxer_metadata.cc: In member function ‘Parse’:
./sample_muxer_metadata.cc:274:19: note: ‘t.minutes’ was declared here