Comment 3 for bug 2051925

Revision history for this message
Christian Ehrhardt  (paelzer) wrote (last edit ):

Review for Source Package: libtracefs

[Summary]
Like many of the kernel associated user space tools and libraries it
is well scoped and does just and exactly what it is supposed to do.
Due to that the packaging, the dependencies and (unit) tests are good too.

We didn't have enough need for it back in bug 2008799, but now we do
as trace-cmd is not useful without. And due to that it will also allow
to make ndctl to have no delta anymore which is a small bonus win.

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 not need a security review

List of specific binary packages to be promoted to main: libtracefs1
This will auto-promote libtracefs-doc and libtracefs-dev which is fine.

Specific binary packages built, but NOT to be promoted to main: n/a

Required TODOs:
- #2 It seems the tests are built by dh_auto_test (we see trace-utest), but
  the tests are not run AFAICS. This will need "build requires root" but
  is worth it. You could as well implement the same in autopkgtest, I'd
  be fine either way.
- #3 Build and use the memory test. There must be a history of "worthwhile
  to check" to have a memory leak test within the makefile. This test
  runs the tests again but within valgrind. We should do so as well in
  our tests. Could be a separate autopkgtest with valgrind installed
  re-runnign the same.

Recommended TODOs:
- #1 I appreciate that at least building the examples is already done in the
  autopkgtests. But if there is a chance to run at least a non exploding
  subset as well in the autopkgtest that would level up the testing
  quite a bit.
  This is just a library, more thought on testing can be done at the
  consumer like trace-cmd which currently just runs list.
  Optional here, but I suggested on trace-cmd (LP: #2051850) to add
  it there as requirement.

I'll assign the bug back to Paul to get those few things resolved.
Once you think you have please un-assign yourself and set it back to "new" state.
We will pick it up in our weekly meeting to hopefully confirm that all is good and ready for promotion to main then.

[Rationale, Duplication and Ownership]
While some tools like perf interact with tracefs as well, there is nothing
in main yet providing this as a maintained abstraction.
=> There is no other package in main providing the same functionality.

A team is committed to own long term maintenance of this package.

The rationale given in the report seems valid and useful for Ubuntu

Problems: None

[Dependencies]
OK:
- just one other Dependencies to MIR due to this, libtraceevent
  filed in bug 2051916
- There is a -dev and -doc package, but they are not aggressively pulling
  in more, no need for exclusion

Problems: None

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

Problems: None

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats (files [images, video, audio,
  xml, json, asn.1], network packets, structures, ...) from
  an untrusted source.
  If we distrust the kernel on a system then we have lost the system already.
- does not expose any external endpoint (port/socket/... or similar)
- 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, ...)
- the use of risk mitigation features at a library is hard as the full
  use case isn't known, ok to not be using things like (dropping permissions,
  using temporary environments, restricted users/groups, seccomp, systemd
  isolation features, apparmor, ...)

Problems: None

Given the clean history and the lack of common exposures I'd judge that
this is ok to continue without explicit security review.

[Common blockers]
OK:
- does not FTBFS currently
- This does not need special HW for build or test
- no new python2 dependency

Problems:
- does have a test suite, but it is build yet not run at build time
- does not have a non-trivial test suite that runs as autopkgtest

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- symbols tracking is in place.
- debian/watch is present and looks ok
- 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
- debian/rules is rather clean
- It is not on the lto-disabled list

Problems: None

[Upstream red flags]
OK:
- no Errors during the build (only a few warnings by recent compilers)
- no incautious use of malloc/sprintf (as far as we can check it),
  but remember the valgrind test case :-)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH (usage is OK inside
  tests)
- no use of user nobody
- no use of setuid / setgid
- not part of the UI for extra checks
- no translation present, but none needed for this case (dev only)

Problems: None