Potential PAM-related memory leak in systemd-executor

[mrc0mmand]

Throughout today I saw two occurrences of a string of PAM-related memory leaks in systemd-executor (#32483 and #32441), in both cases they were hit by dfuzzer:

(sh)[1334]: Direct leak of 384 byte(s) in 1 object(s) allocated from:
(sh)[1334]:     #0 0x712022f5bcf1 in calloc (/usr/lib/clang/17/lib/linux/libclang_rt.asan-x86_64.so+0x15bcf1) (BuildId: beb0a35f2d0823fd6420f355da05e49e71d737f5)
(sh)[1334]:     #1 0x712023a0662b  (/usr/lib/libpam.so.0+0x762b) (BuildId: de2d275cb2a197c9435d80f888b8b56829fc0ec3)
(sh)[1334]:     #2 0x5b1b37cbab9f in setup_pam /systemd-meson-build/../build/src/core/exec-invoke.c:1126:20
(sh)[1334]:     #3 0x5b1b37c85ffb in exec_invoke /systemd-meson-build/../build/src/core/exec-invoke.c:4687:21
(sh)[1334]:     #4 0x5b1b37c7064d in run /systemd-meson-build/../build/src/core/executor.c:236:13
(sh)[1334]:     #5 0x5b1b37c7064d in main /systemd-meson-build/../build/src/core/executor.c:267:13
(sh)[1334]:     #6 0x712020c43ccf  (/usr/lib/libc.so.6+0x25ccf) (BuildId: 6542915cee3354fbcf2b3ac5542201faec43b5c9)

Full report: https://gist.githubusercontent.com/mrc0mmand/07fd8ec0e6651f3f0f18bfaa1c984f03/raw/10fa41c289a879b5990d5038c0f16909bc6b74e9/gistfile1.txt

Logs from affected jobs:

• https://jenkins-systemd.apps.ocp.cloud.ci.centos.org/job/upstream-vagrant-archlinux-sanitizers/13327/artifact//systemd-centos-ci/index.html
• https://jenkins-systemd.apps.ocp.cloud.ci.centos.org/job/upstream-vagrant-archlinux-sanitizers/13265/artifact//systemd-centos-ci/index.html

I guess #32441 is the culprit, as it was the first one to hit this and also touches systemd-executor stuff.

[dtardon]

@ldv-alt Any idea?

[keszybz]

I looked at #32441 and it doesn't seem to do any allocations that could be leaked. Based on the call stack, this seems to be an issue in PAM.

[ldv-alt]

Since the parent process is intentionally not calling pam_end, the pam handle and all memory it references are leaked.

[poettering]

Hmm, so in case of PAM session stuff we fork() off a process that sticks around and maintains the PAM session. it's a copy of process we execute because of fork(), and that makes it really weird to do clean-ups since many libraries (and not even our own code, i.e. various sd-* libs) is happy to clean up things in both child and parent.

Hence, this is kinda intended.

I figure we can improve things a bit though:

Right now we fork(), and let the child keep the pam session open, and execve() the actual payload in the parent. This is a really unusual setup, and probably many pam modules don't like this very much. We do this because we want that the main process of the daemon is always the process that PID 1 forks off, so that we can watch SIGCHLD. And thus it seemed smart to fork off the session destructor as a child, back in the day.

We should look into normalizing this somewhat, after all we nowadays happily swap out main processes during service runtime via the MAINPID= sd_notify() logic. Hence we could do so here too. Hence maybe in PID 1 create an AF_UNIX socket (maybe speaking varlink, to make things easy) that we use for communication with the PAM session stub process. Then, switch around the roles in the fork(), and let PID 1 know about the child via the AF_UNIX socket (so that it updtaes the main pid of the service), and then just wait until the child dies, and report its exit status also via that AF_UNIX socket.

That should give us pretty OK behaviour: the ordering is normalized, but we lose no tracking information.

But either way, this is not something for v256, it's major work, and this leak isn't really a relevant leak, but kinda expected.