[MIR] promote libtraceevent as a trace-cmd dependency

 % lintian --pedantic
W: libtraceevent source: newer-standards-version 4.6.2 (current is 4.6.0.1)
P: libtraceevent source: no-homepage-field
P: libtraceevent source: silent-on-rules-requiring-root [debian/control]
P: libtraceevent source: update-debian-copyright 2023 vs 2024 [debian/copyright:31]

Review for Source Package: libtraceevent

[Summary]
There are quite some details to clarify before giving a MIR ACK or NACK. Some were already coming from the previous MIR and are not addressed before opening this new MIR. If we want to be able to maintain it properly, we really need to address them beforehand.

Also, this does need a security review, so I'll assign ubuntu-security once the other issues are fixed: there are a lot of malloc’s use and it’s working with trusted kernel space. I think a security review will help here.
List of specific binary packages to be promoted to main: libtraceevent-dev, libtraceevent-doc, libtraceevent1, libtraceevent1-plugin

Some parts are going to paraphrase what was written in the previous MIR request.

Required TODOs:
- It does not have a test suite that runs at build time: upstream's unit tests
  from utest/ are being compiled via `make test` but don't seem to be executed.
  I cannot spot anything outside of the compilation content output in the build log.
- Similarly, as written by the report, it does not have a non-trivial test suite that runs
  as autopkgtest: it checks compilation only, I think similarly than the previous request, we should
  at least test the unit tests against the installed version.
- there are a lot of "#MISSING: ..." entries in .symobls tracking. Can we get a comment on any of those?
- The package should get a team bug subscriber before being promoted

Recommended TODOs:
- One warning is repeated throughout the build (install target) of the software:
  `install: WARNING: ignoring --strip-program option as -s option was not specified`.
  Can we fix it so that it doesn’t obfuscate other warnings?
- The package is shipping a .a file, we generally strip those in ubuntu, do we really need this static archive?

[Rationale, Duplication and Ownership]
There is no other package in main providing the same functionality.
Foundation team is committed to own long term maintenance of this package.
TODO: The rationale given in the report seems valid and useful for Ubuntu

[Dependencies]
OK:
- no other Dependencies to MIR due to this
  - libtraceevent 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:
- 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

[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.
- does not expose any external endpoint (port/socket/... or similar)
- does not process arbitrary web content
- does not ...

Hey didrocks, thanks for the review.

About the `# MISSING` entries in libtraceevent1.symbols, this is a bit unclear to me what is expected. I checked with sudipmuk and looked at the commit history, these are symbols now removed from the lib. So I understand that documenting it this way is the proper way.

I also checked every reverse-dependency of the lib, looked for the symbols marked MISSING and confirmed none of them is currently used (at least in noble). So we could remove these entries.

But I do not really understand the harm of having these entries kept for documentation, except this could pile up and become a mess at some point. Do we have a policy regarding the removal of these entries (count of version, age)?

sudipmuk confirmed they will remove those #MISSING comments on the next update.

Here is a patch to run the test suite when building.

See this buildlog https://launchpadlibrarian.net/715275635/buildlog_ubuntu-noble-amd64.libtraceevent_1%3A1.8.2-1ubuntu1_BUILDING.txt.gz from my test PPA executing the test suite.

> we should at least test the unit tests against the installed version

I am not sure how to do that. Currently the unit tests use the libtraceevent.a generated at build time. So I suppose it should not really make any difference to run it in autopkgtest or at build time (see my patch in comment #5).

I thought about copying utest/Makefile in debian/tests/, replacing LIBTRACEEVENT_STATIC with the path to the installed libtraceevent.so and call "make test; ./trace-utest" in debian/tests/control.

I also thought about calling `./test-event -f /sys/kernel/tracing/events/ftrace/print/format` once the sample is built.

But I do not know if any of these methods would be "hacky" or good enough.

By the way there is the same issue on libtracefs so I guess finding a method fitting both packages could be easier to maintain.

Some examples of how this has been done in the past are:
* https://git.launchpad.net/ubuntu/+source/libei/tree/debian/tests
* https://salsa.debian.org/utopia-team/libbytesize/-/merge_requests/3/diffs

Here is a patch to run utest at build time and build+run it with the installed lib in autopkgtest.

I will update with the log of a successful autopkgtest run once this is done.

The ppa: https://launchpad.net/~upils/+archive/ubuntu/test-ppa/+packages

  - libtraceevent/1:1.8.2-1ubuntu3
    + ✅ libtraceevent on noble for amd64 @ 26.02.24 16:51:31
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/amd64/libt/libtraceevent/20240226_165131_ef945@/log.gz
    + ✅ libtraceevent on noble for arm64 @ 27.02.24 09:33:37
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/arm64/libt/libtraceevent/20240227_093337_db8d7@/log.gz
    + ✅ libtraceevent on noble for armhf @ 27.02.24 09:32:27
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/armhf/libt/libtraceevent/20240227_093227_881ff@/log.gz
    + ✅ libtraceevent on noble for ppc64el @ 27.02.24 09:27:18
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/ppc64el/libt/libtraceevent/20240227_092718_429bf@/log.gz
    + ✅ libtraceevent on noble for s390x @ 27.02.24 09:26:53
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/s390x/libt/libtraceevent/20240227_092653_815df@/log.gz

Even though the test on s390x pass, looking at the test suite resuls show it should fail. I need to understand why this failure is not properly reported.

Updated patch. The test binary will now return 1 if at least one test failed.

I will run autopkgtest when the pkg is published, and I expect it will fail (and correctly report this failure) for s390x. We should then decide if this failure is blocking the MIR process or not.

The ppa: https://launchpad.net/~upils/+archive/ubuntu/test-ppa/+packages

  - libtraceevent/1:1.8.2-1ubuntu5
    + ✅ libtraceevent on noble for amd64 @ 27.02.24 14:45:01
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/amd64/libt/libtraceevent/20240227_144501_533ef@/log.gz
    + ✅ libtraceevent on noble for arm64 @ 27.02.24 14:44:50
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/arm64/libt/libtraceevent/20240227_144450_c5fe0@/log.gz
    + ✅ libtraceevent on noble for armhf @ 27.02.24 14:50:27
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/armhf/libt/libtraceevent/20240227_145027_856fe@/log.gz
    + ✅ libtraceevent on noble for ppc64el @ 27.02.24 14:50:53
      • Log: https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/ppc64el/libt/libtraceevent/20240227_145053_65299@/log.gz

I have opened a dedicated bug to work on the patch as discussed with slyon. See LP: #2055258

I now have a patch adding (and fixing) tests running at build and in autopkgtest. See https://bugs.launchpad.net/ubuntu/+source/libtraceevent/+bug/2055258/comments/11

I am now waiting for sponsorship on this upload.

A few comments after a first look at the patch:

1. You should include bug numbers in the changelog, and in the relevant patch files (using the Bug-Ubuntu dep3 field).
2. The version string is incorrect. The previous upload is 1:1.8.2-1, so this should be 1:1.8.2-1ubuntu1. It's also generally good to set the release field instead of leaving UNRELEASED, even though a sponsor could easily change this.
3. On this Makefile diff:

+--- a/Makefile
++++ b/Makefile
+@@ -257,6 +257,10 @@
+ test: force $(LIBTRACEEVENT_STATIC)
+ $(Q)$(call descend,$(UTEST_DIR),test)
+
++test-installed: LIBTRACEEVENT_STATIC := $(shell dpkg -L libtraceevent-dev | grep libtraceevent.so)

Shouldn't this be libtraceevent.a? Further, wouldn't it be better to make this `LIBTRACEEVENT_STATIC = -l:libtraceevent.a`?

++test-installed: force
++ $(Q)$(call descend,$(UTEST_DIR),test)
++
+ VIM_TAGS = $(obj)/tags
+ EMACS_TAGS = $(obj)/TAGS
+

I think it would also be simpler to make this patch do:

diff --git a/Makefile b/Makefile
index 41ad866..8ffc529 100644
--- a/Makefile
+++ b/Makefile
@@ -114,7 +114,7 @@ EXTRAVERSION = $(EP_EXTRAVERSION)
 OBJ = $@
 N =

-LIBTRACEEVENT_STATIC = $(bdir)/libtraceevent.a
+LIBTRACEEVENT_STATIC ?= $(bdir)/libtraceevent.a
 LIBTRACEEVENT_SHARED = $(bdir)/libtraceevent.so.$(EVENT_PARSE_VERSION)

 EP_HEADERS_DIR = $(src)/include/traceevent

and then in debian/tests/utest, you can just call LIBTRACEEVENT_STATIC='-l:libtraceevent.a'. This Makefile patch has the advantage of probably being very upstream-able too.

Thanks for the review enr0n!

I like your solution of setting LIBTRACEEVENT_STATIC in debian/tests/utest to have minimal changes in the Makefile.

> Shouldn't this be libtraceevent.a? Further, wouldn't it be better to make this `LIBTRACEEVENT_STATIC = -l:libtraceevent.a`?

In its review didrocks specifically asked to run the tests with the installed library. That is why I am looking for its path and using it to replace LIBTRACEEVENT_STATIC when building the test binary. Does it make sense? I am missing something?

Is that what you had in mind? Because it looks like this is working as I expected

See https://autopkgtest.ubuntu.com/results/autopkgtest-noble-upils-test-ppa/noble/amd64/libt/libtraceevent/20240311_132302_36e50@/log.gz

Did I miss something?

Passing `-l:libtraceevent.a` *will* use the installed library, unless it's installed in some odd location outside of LIBRARY_PATH, which does not appear to be the case. This is doing the same thing as any other `-l<libname>` usage, but specifies that we specifically want the static version of this library.

Your patch still searches for the shared object and assigns that to LIBRARY_STATIC. This looks wrong. Are you intentionally populating LIBRARY_STATIC with a path to a *shared* library?

The version number for the package also looks wrong still. Finally, as a note of organization, since you are making several changes related to one bug, I would write your changelog entry along the lines of:

* <Write your summary here> (LP: #2055258)
  - Build but also run the test suite when building the pkg.
  - Run unit test as autopkgtest
  - Fix test running on big endian arch

> The version number for the package also looks wrong still.

Yes, because for now I am submitting the patch I am generating from the build I submit to my PPA to test. I will fix this once everything is done.

> Are you intentionally populating LIBRARY_STATIC with a path to a *shared* library?

Yes, this is done intentionally because AFAIU the static lib is not supposed to be used by consumers of libtraceevent (and could even not be included in the package). So I thought it best to build the test binary using the installed shared library. But I may totally be wrong on that so if using the static one is fine I can fix it!

> I would write your changelog entry along the lines of

Yes, good point, it looks much better!

> Yes, because for now I am submitting the patch I am generating from the build I submit to my PPA to test.

FWIW, the convention for ppa uploads is to append ~ppaX to the new version number.

> Yes, this is done intentionally because AFAIU the static lib is not supposed to be used by consumers of libtraceevent

Ah, okay. This was causing me a lot of confusion, since you are kind of mis-using the LIBTRACEEVENT_STATIC variable. The point remains either way that you should simply use -ltraceevent (this is what any real consumer will do) instead of grepping `dpkg -L`. If you want to keep using LIBTRACEEVENT_STATIC for this purpose, please add a comment explaining that we are knowingly hijacking this variable to instead link against the shared library. Otherwise, it will continue to be confusing.

FTR: There was an older MIR discussion around libtraceevent in bug #2009715 (concerns were similar).

> FWIW, the convention for ppa uploads is to append ~ppaX to the new version number.

Oh my bad, I forgot about that!

> This was causing me a lot of confusion

Sorry about that, it was not clearly mentioned.

> If you want to keep using LIBTRACEEVENT_STATIC for this purpose,

I did this because I thought it would prevent from making too much changes in the makefile and would be maintainable. I will do the fixes you suggested.

I reviewed libtraceevent 1:1.8.2-1 as checked into noble. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

> libtraceevent - Linux kernel trace event library

- CVE History:
  - none
- Build-Depends?
  - nothing concerning
  - most dependencies are for building documentation
- pre/post inst/rm scripts?
  - none
- init scripts?
  - none
- systemd units?
  - none
- dbus services?
  - none
- setuid binaries?
  - none
- binaries in PATH?
  - none
- sudo fragments?
  - none
- polkit files?
  - none
- udev rules?
  - none
- cron jobs?
  - none
- unit tests / autopkgtests?
  - in progress by owning team
- Build logs:
  - missing MAN pages
    - documentation warnings make build logs noisy
  - W: libtraceevent source: build-depends-on-obsolete-package Build-Depends: pkg-config => pkgconf

- Processes spawned?
  - ./src/parse-filter.c runs regexec
    - this is a library, secure implementation depends on downstream projects
- Memory management?
  - heavy use
    - care seems to be taken
    - as a root process, bugs are unlikely to cause vulnerabilities
    - this is a library, secure implementation depends on downstream projects
- File IO?
  - load_plugin() from ./src/event-plugin.c use dlopen
    - security depends on how downstream projects load plugins
    - assume plugins are root
- Logging?
  - contains error handling messages
  - mostly in ./src/parse-filter.c
- Environment variable usage?
  - TRACEEVENT_PLUGIN_DIR
  - HOME
- Use of privileged functions?
  - none
- Use of cryptography / random number sources etc?
  - none
- Use of temp files?
  - none
- Use of networking?
  - minimal use in ./src/event-parse.c
- Use of WebKit?
  - none
- Use of PolicyKit?
  - none

- Any significant cppcheck and Coverityresults?
  - false positives
    - these looked relevant at first glance, but not after analysis
- Any significant shellcheck results?
  - none, all reports are for manpages/tests/building
- Any significant bandit results?
  - none

Security team ACK for promoting libtraceevent to main.

I uploaded the patch in bug 2055258 for Paul, which addresses the TODOs about build time tests and autopkgtest.

I just subscribed the ~foundations-bugs team. Security review looking good (comment #20).

I can now see proper autopkgtests and "dh_auto_test" during build.

The "#MISSING: " lines in libtraceevent1.symbols have been explained in comment #3 & #4 after discussions with the upstream/Debian maintainer.

So it looks like all MIR requirements from comment #2 are addressed. LGTM. @didrocks WDYT?

We agreed that the "#MISSING: " lines will be downgraded to a Recommended MIR TODO.

<slyon> Do you want those #MISSING: symbols dropped? IMO it should be fine as-is. Upstream is aware and want's to drop it anyway
<cpaelzer> #MISSING is fine unless it keeps adding more and more and more of them

So this should be good to go!

Hey everyone and Paul. First, sorry for the delayed answered (I was thinking you would get me reassign and for some reason, I missed subscribing to the bug)

> But I do not really understand the harm of having these entries kept for documentation, except this could pile up and become a mess at some point. Do we have a policy regarding the removal of these entries (count of version, age)?

There is no strict policy, I understand the historical part of having it for documenting. I suggest to keep it for some release, but if this is doable, cleanup after a while. It’s not something we want to keep hanging around forever. I see that you want to remove them in a future upload, good!

All the required TODOs are now fullfilled, thanks for working on those! I’m thus happy to MIR ack this package now!

Override component to main
libtraceevent 1:1.8.2-1ubuntu2 in noble: universe/misc -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble amd64: universe/libdevel/optional/100% -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble arm64: universe/libdevel/optional/100% -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble armhf: universe/libdevel/optional/100% -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble ppc64el: universe/libdevel/optional/100% -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble riscv64: universe/libdevel/optional/100% -> main
libtraceevent-dev 1:1.8.2-1ubuntu2 in noble s390x: universe/libdevel/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble amd64: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble arm64: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble armhf: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble i386: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble ppc64el: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble riscv64: universe/doc/optional/100% -> main
libtraceevent-doc 1:1.8.2-1ubuntu2 in noble s390x: universe/doc/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble amd64: universe/libs/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble arm64: universe/libs/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble armhf: universe/libs/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble ppc64el: universe/libs/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble riscv64: universe/libs/optional/100% -> main
libtraceevent1 1:1.8.2-1ubuntu2 in noble s390x: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble amd64: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble arm64: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble armhf: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble ppc64el: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble riscv64: universe/libs/optional/100% -> main
libtraceevent1-plugin 1:1.8.2-1ubuntu2 in noble s390x: universe/libs/optional/100% -> main
Override [y|N]? y
26 publications overridden.