From 3ad9149ec6a3015860e712d8ac1c40470a59ec07 Mon Sep 17 00:00:00 2001 From: Zeyad Yasser Date: Tue, 28 Oct 2025 11:12:00 +0300 Subject: [PATCH 01/10] many: reintroduce fdstore helpers This commit reintroduces reverted PR #16119 * systemd: add fdstore helpers The initial use case of using systemd's fdstore is to keep sensitive data that cannot be persisted to disk like recovery keys and passphrases while having them survive snapd restarts to increase the robustness of FDE operation. * netutil: only use activation sockets passed from systemd In upcoming work, more fds will be passed from systemd on startup, previously the helper from go-system considered all passed fds as activation fds which will not be true in the future. * packaging: remove go-systemd dependency This was the only usage of go-systemd, so I am removing its dependency as well. --------- Signed-off-by: Zeyad Gouda --- go.mod | 1 - go.sum | 2 - netutil/activation.go | 23 +-- packaging/debian-sid/control | 1 - packaging/fedora/snapd.spec | 3 - systemd/fdstore/export_test.go | 64 +++++++ systemd/fdstore/fdstore.go | 283 ++++++++++++++++++++++++++++++ systemd/fdstore/fdstore_test.go | 297 ++++++++++++++++++++++++++++++++ 8 files changed, 656 insertions(+), 18 deletions(-) create mode 100644 systemd/fdstore/export_test.go create mode 100644 systemd/fdstore/fdstore.go create mode 100644 systemd/fdstore/fdstore_test.go diff --git a/go.mod b/go.mod index ce7ed96d56b..e2559d90ad6 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,6 @@ require ( github.com/canonical/go-sp800.90a-drbg v0.0.0-20210314144037-6eeb1040d6c3 // indirect github.com/canonical/go-tpm2 v1.15.0 github.com/chai2010/gettext-go v1.0.3 - github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf github.com/godbus/dbus/v5 v5.1.0 github.com/gorilla/mux v1.8.0 github.com/gvalkov/golang-evdev v0.0.0-20191114124502-287e62b94bcb diff --git a/go.sum b/go.sum index 2a132ac8025..714a28e481e 100644 --- a/go.sum +++ b/go.sum @@ -16,8 +16,6 @@ github.com/canonical/tcglog-parser v0.0.0-20240924110432-d15eaf652981 h1:vrUzSfb github.com/canonical/tcglog-parser v0.0.0-20240924110432-d15eaf652981/go.mod h1:ywdPBqUGkuuiitPpVWCfilf2/gq+frhq4CNiNs9KyHU= github.com/chai2010/gettext-go v1.0.3 h1:9liNh8t+u26xl5ddmWLmsOsdNLwkdRTg5AG+JnTiM80= github.com/chai2010/gettext-go v1.0.3/go.mod h1:y+wnP2cHYaVj19NZhYKAwEMH2CI1gNHeQQ+5AjwawxA= -github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf h1:iW4rZ826su+pqaw19uhpSCzhj44qo35pNgKFGqzDKkU= -github.com/coreos/go-systemd v0.0.0-20191104093116-d3cd4ed1dbcf/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/frankban/quicktest v1.2.2 h1:xfmOhhoH5fGPgbEAlhLpJH9p0z/0Qizio9osmvn9IUY= diff --git a/netutil/activation.go b/netutil/activation.go index 9082e6bb207..19f76c2d483 100644 --- a/netutil/activation.go +++ b/netutil/activation.go @@ -24,9 +24,8 @@ import ( "net" "os" - "github.com/coreos/go-systemd/activation" - "github.com/snapcore/snapd/logger" + "github.com/snapcore/snapd/systemd/fdstore" ) // GetListener tries to get a listener for the given socket path from the @@ -74,17 +73,19 @@ func GetListener(socketPath string, listenerMap map[string]net.Listener) (net.Li // ActivationListeners builds a map of addresses to listeners that were passed // during systemd activation func ActivationListeners() (lns map[string]net.Listener, err error) { - // pass false to keep LISTEN_* environment variables passed by systemd - files := activation.Files(false) - lns = make(map[string]net.Listener, len(files)) + socketFds := fdstore.ActivationSocketFds() - for _, f := range files { - ln, err := net.FileListener(f) - if err != nil { - return nil, err + lns = make(map[string]net.Listener, len(socketFds)) + for name, fds := range socketFds { + for _, fd := range fds { + f := os.NewFile(uintptr(fd), name) + ln, err := net.FileListener(f) + if err != nil { + return nil, err + } + addr := ln.Addr().String() + lns[addr] = ln } - addr := ln.Addr().String() - lns[addr] = ln } return lns, nil } diff --git a/packaging/debian-sid/control b/packaging/debian-sid/control index 9c0889451e3..b077a6e96d3 100644 --- a/packaging/debian-sid/control +++ b/packaging/debian-sid/control @@ -22,7 +22,6 @@ Build-Depends: debhelper (>= 13), golang-github-bmatcuk-doublestar-dev, golang-github-chai2010-gettext-go-dev, golang-github-coreos-bbolt-dev, - golang-github-coreos-go-systemd-dev, golang-github-gorilla-mux-dev, golang-github-jessevdk-go-flags-dev, golang-github-juju-ratelimit-dev, diff --git a/packaging/fedora/snapd.spec b/packaging/fedora/snapd.spec index b191ec8052b..2de125894cb 100644 --- a/packaging/fedora/snapd.spec +++ b/packaging/fedora/snapd.spec @@ -179,7 +179,6 @@ Provides: %{name}-login-service%{?_isa} = 1.33 %if ! 0%{?with_bundled} BuildRequires: golang(github.com/bmatcuk/doublestar/v4) BuildRequires: golang(github.com/chai2010/gettext-go) -BuildRequires: golang(github.com/coreos/go-systemd/activation) BuildRequires: golang(github.com/godbus/dbus/v5) BuildRequires: golang(github.com/godbus/dbus/v5/introspect) BuildRequires: golang(github.com/gorilla/mux) @@ -277,7 +276,6 @@ BuildArch: noarch %if ! 0%{?with_bundled} Requires: golang(github.com/bmatcuk/doublestar/v4) Requires: golang(github.com/chai2010/gettext-go) -Requires: golang(github.com/coreos/go-systemd/activation) Requires: golang(github.com/godbus/dbus/v5) Requires: golang(github.com/godbus/dbus/v5/introspect) Requires: golang(github.com/gorilla/mux) @@ -309,7 +307,6 @@ Requires: golang(gopkg.in/yaml.v3) # *sigh*... I hate golang... Provides: bundled(golang(github.com/bmatcuk/doublestar/v4)) Provides: bundled(golang(github.com/chai2010/gettext-go)) -Provides: bundled(golang(github.com/coreos/go-systemd/activation)) Provides: bundled(golang(github.com/godbus/dbus/v5)) Provides: bundled(golang(github.com/godbus/dbus/v5/introspect)) Provides: bundled(golang(github.com/gorilla/mux)) diff --git a/systemd/fdstore/export_test.go b/systemd/fdstore/export_test.go new file mode 100644 index 00000000000..4f3a71bde3a --- /dev/null +++ b/systemd/fdstore/export_test.go @@ -0,0 +1,64 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2025 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +package fdstore + +import ( + "github.com/snapcore/snapd/testutil" +) + +func MockOsGetenv(f func(key string) string) (restore func()) { + return testutil.Mock(&osGetenv, f) +} + +func MockOsUnsetenv(f func(key string) error) (restore func()) { + return testutil.Mock(&osUnsetenv, f) +} + +func MockOsLookupEnv(f func(key string) (string, bool)) (restore func()) { + return testutil.Mock(&osLookupEnv, f) +} + +func MockOsGetpid(f func() int) (restore func()) { + return testutil.Mock(&osGetpid, f) +} + +func MockUnixClose(f func(fd int) (err error)) (restore func()) { + return testutil.Mock(&unixClose, f) +} + +func MockUnixCloseOnExec(f func(fd int)) (restore func()) { + return testutil.Mock(&unixCloseOnExec, f) +} + +func MockSdNotify(f func(notifyState string) error) (restore func()) { + return testutil.Mock(&sdNotify, f) +} + +func MockSdNotifyWithFds(f func(notifyState string, fds ...int) error) (restore func()) { + return testutil.Mock(&sdNotifyWithFds, f) +} + +func KnownFdNames() map[FdName]bool { + return knownFdNames +} + +func Clear() { + fdstore = nil +} diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go new file mode 100644 index 00000000000..a64c7c2a413 --- /dev/null +++ b/systemd/fdstore/fdstore.go @@ -0,0 +1,283 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2025 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +package fdstore + +import ( + "fmt" + "os" + "strconv" + "strings" + "sync" + + "github.com/snapcore/snapd/logger" + "github.com/snapcore/snapd/strutil" + "github.com/snapcore/snapd/systemd" + "golang.org/x/sys/unix" +) + +const sd_LISTEN_FDS_START = 3 + +type FdName string + +const ( + FdNameMemfdSecretState FdName = "memfd-secret-state" +) + +var knownFdNames = map[FdName]bool{ + FdNameMemfdSecretState: true, +} + +func (name FdName) validate() error { + if !name.isSocket() && !knownFdNames[name] { + return fmt.Errorf(`unknown file descriptor name %q`, name) + } + return nil +} + +func (name FdName) isSocket() bool { + return strings.HasSuffix(string(name), ".socket") +} + +var ( + osGetenv = os.Getenv + osUnsetenv = os.Unsetenv + osLookupEnv = os.LookupEnv + osGetpid = os.Getpid + unixClose = unix.Close + unixCloseOnExec = unix.CloseOnExec + sdNotify = systemd.SdNotify + sdNotifyWithFds = systemd.SdNotifyWithFds +) + +var fdstore map[FdName][]int +var mu sync.RWMutex + +func initFdstore() { + mu.Lock() + defer mu.Unlock() + + if fdstore != nil { + // fdstore map is lazily loaded once + return + } + + // Make sure initialization only happens once, only here. + defer func() { + osUnsetenv("LISTEN_PID") + osUnsetenv("LISTEN_FDS") + osUnsetenv("LISTEN_FDNAMES") + }() + + // Initialize fdstore before any processing so + // it is only done once. + fdstore = make(map[FdName][]int) + + pid, err := strconv.Atoi(osGetenv("LISTEN_PID")) + if err != nil || pid != osGetpid() { + return + } + + nfds, err := strconv.Atoi(osGetenv("LISTEN_FDS")) + if err != nil || nfds == 0 { + return + } + + var names []string + namesEnv, namesEnvExists := osLookupEnv("LISTEN_FDNAMES") + if namesEnvExists { + names = strings.Split(namesEnv, ":") + } else { + // Likely old systemd <227 (e.g. amazon-linux-2), Assume all passed + // fds are activation sockets as a fallback. + names = make([]string, nfds) + for i := 0; i < nfds; i++ { + // A generic name with .socket suffix is enough + // to be picked up by ActivationSocketFds. + names[i] = fmt.Sprintf("activation-fd-%d.socket", i) + } + } + + if len(names) != nfds { + logger.Noticef("internal error: cannot initialize fdstore: $LISTEN_FDNAMES does not match $LISTEN_FDS") + return + } + + for i := 0; i < nfds; i++ { + fd := sd_LISTEN_FDS_START + i + name := FdName(names[i]) + fdstore[name] = append(fdstore[name], fd) + + // TODO: Use raw fcntl and check for errors. + unixCloseOnExec(fd) + } + + // Prune unexpected file descriptors + for name, fds := range fdstore { + shouldRemove := false + if err := name.validate(); err != nil { + logger.Noticef("unexpected fdstore entry %q found: %v", name, err) + shouldRemove = true + } + // Only activation sockets can be associated with multiple fds. + if !name.isSocket() && len(fds) != 1 { + logger.Noticef("unexpected fdstore entry %[1]q found: %[1]q has more than one fd", name) + shouldRemove = true + } + if shouldRemove { + logger.Noticef("removing unexpected fdstore entry %q", name) + if err := removeUnlocked(name); err != nil { + logger.Noticef("internal error: cannot remove fdstore entry %q: %v\n", name, err) + continue + } + } + } + + return +} + +// Remove removes file descriptors from systemd given their name. +// Remove cannot remove activation sockets. +func Remove(name FdName) (err error) { + initFdstore() + + if name.isSocket() { + // Activation sockets can only be passed down from systemd + // i.e. file descriptors whose name has a ".socket" suffix + return fmt.Errorf("cannot remove file descriptor from fdstore: sockets cannot be removed") + } + + mu.Lock() + defer mu.Unlock() + return removeUnlocked(name) +} + +func removeUnlocked(name FdName) (err error) { + // FDSTOREREMOVE=1 was added in systemd v236 + // + // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDSTOREREMOVE=1 + if err := systemd.EnsureAtLeast(236); err != nil { + return fmt.Errorf("cannot remove file descriptor from fdstore: %v", err) + } + + state := fmt.Sprintf("FDSTOREREMOVE=1\nFDNAME=%s", name) + if err := sdNotify(state); err != nil { + return err + } + + var closeErrs []error + for _, fd := range fdstore[name] { + if err := unixClose(fd); err != nil { + // record error and keep going + closeErrs = append(closeErrs, err) + } + } + + delete(fdstore, name) + return strutil.JoinErrors(closeErrs...) +} + +// Get retrieves file descriptors passed from systemd by their name. +// close-on-exec is set on the returned file descriptor. -1 is returned +// if no matching file descriptor is found or if the passed name +// corresponds to a socket (i.e. ends in ".socket"). To get activation +// sockets use fdstore.ActivationSocketFds() instead. +func Get(name FdName) (fd int) { + initFdstore() + + mu.RLock() + defer mu.RUnlock() + + if name.isSocket() { + // Activation socket file descriptors should be accessed + // through ActivationSocketFds. + return -1 + } + + fds := fdstore[name] + if len(fds) != 1 { + return -1 + } + return fds[0] +} + +// Add passes a file descriptor to systemd associated with a name +// to reuse it across snapd restarts. +// +// - The file descriptors can be retrieved by calling Get(). +// - Only a single file descriptor can associated with a FdName. +func Add(name FdName, fd int) error { + initFdstore() + + // FDNAME=... was added in systemd v233, but for the sake + // of being consistent with removal (FDSTOREREMOVE=1 was + // added in systemd v236), require at least systemd v236. + // + // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDNAME=%E2%80%A6 + if err := systemd.EnsureAtLeast(236); err != nil { + return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) + } + + mu.Lock() + defer mu.Unlock() + + if err := name.validate(); err != nil { + return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) + } + if name.isSocket() { + // Activation sockets can only be passed down from systemd + // i.e. file descriptors whose name has a ".socket" suffix + return fmt.Errorf("cannot add file descriptor to fdstore: sockets are not allowed") + } + if len(fdstore[name]) != 0 { + return fmt.Errorf("cannot add file descriptor to fdstore: %q already exists", name) + } + + state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) + if err := sdNotifyWithFds(state, fd); err != nil { + return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) + } + + fdstore[name] = []int{fd} + return nil +} + +// ActivationSocketFds returns activation socket file descriptors +// that were passed from systemd. Only sockets whose name has a +// ".socket" suffix are returned. +func ActivationSocketFds() (socketFds map[string][]int) { + initFdstore() + + mu.RLock() + defer mu.RUnlock() + + socketFds = make(map[string][]int) + // The file descriptor name defaults to the name of the socket + // unit (including its .socket suffix), unless it was explicitly + // assigned by setting `FileDescriptorName=` on the socket unit. + // + // `FileDescriptorName=` was added in systemd version 227. + for name, fds := range fdstore { + if name.isSocket() { + socketFds[string(name)] = append(socketFds[string(name)], fds...) + } + } + + return socketFds +} diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go new file mode 100644 index 00000000000..bff54e2a820 --- /dev/null +++ b/systemd/fdstore/fdstore_test.go @@ -0,0 +1,297 @@ +// -*- Mode: Go; indent-tabs-mode: t -*- + +/* + * Copyright (C) 2025 Canonical Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 3 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +package fdstore_test + +import ( + "errors" + "fmt" + "testing" + + . "gopkg.in/check.v1" + + "github.com/snapcore/snapd/strutil" + "github.com/snapcore/snapd/systemd" + "github.com/snapcore/snapd/systemd/fdstore" + "github.com/snapcore/snapd/testutil" +) + +// Hook up check.v1 into the "go test" runner +func Test(t *testing.T) { TestingT(t) } + +type fdstoreTestSuite struct { + testutil.BaseTest + + fakeEnv map[string]string + sdNotifyCalls []string + errOn []string + closedFds []int + closeOnExecFds []int +} + +var _ = Suite(&fdstoreTestSuite{}) + +func (s *fdstoreTestSuite) SetUpTest(c *C) { + s.fakeEnv = map[string]string{"LISTEN_PID": "1984"} + s.sdNotifyCalls = nil + s.errOn = nil + s.closedFds = nil + s.closeOnExecFds = nil + + s.AddCleanup(fdstore.MockOsGetenv(func(key string) string { + return s.fakeEnv[key] + })) + s.AddCleanup(fdstore.MockOsUnsetenv(func(key string) error { + delete(s.fakeEnv, key) + return nil + })) + s.AddCleanup(fdstore.MockOsLookupEnv(func(key string) (string, bool) { + val, exists := s.fakeEnv[key] + return val, exists + })) + s.AddCleanup(fdstore.MockOsGetpid(func() int { + return 1984 + })) + s.AddCleanup(fdstore.MockSdNotify(func(notifyState string) error { + call := fmt.Sprintf("sd-notify: %s", notifyState) + if strutil.ListContains(s.errOn, call) { + return errors.New("boom!") + } + s.sdNotifyCalls = append(s.sdNotifyCalls, call) + return nil + })) + s.AddCleanup(fdstore.MockSdNotifyWithFds(func(notifyState string, fds ...int) error { + call := fmt.Sprintf("sd-notify-with-fds: %s %v", notifyState, fds) + if strutil.ListContains(s.errOn, call) { + return errors.New("boom!") + } + s.sdNotifyCalls = append(s.sdNotifyCalls, call) + return nil + })) + s.AddCleanup(fdstore.MockUnixClose(func(fd int) (err error) { + if strutil.ListContains(s.errOn, fmt.Sprintf("close-fd: %d", fd)) { + return errors.New("boom!") + } + s.closedFds = append(s.closedFds, fd) + return nil + })) + s.AddCleanup(fdstore.MockUnixCloseOnExec(func(fd int) { + s.closeOnExecFds = append(s.closeOnExecFds, fd) + })) + s.AddCleanup(systemd.MockSystemdVersion(236, nil)) + s.AddCleanup(fdstore.Clear) +} + +func (s *fdstoreTestSuite) TestGet(c *C) { + s.fakeEnv["LISTEN_FDS"] = "5" + s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:invalid:snapd.socket:memfd-secret-state:snapd.socket" + // fds starts from 3 + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 6) + + // fdstore is lazily initialized once, and clears passed environment + c.Assert(s.fakeEnv, HasLen, 0) + + // more checks + c.Check(fdstore.Get("no-fd"), Equals, -1) // doesn't exist + c.Check(fdstore.Get("invalid"), Equals, -1) // should have been pruned by initialization + c.Check(fdstore.Get("snapd.socket"), Equals, -1) // sockets are not returned + + // check remove call for "invalid" fd + c.Check(s.sdNotifyCalls, DeepEquals, []string{ + "sd-notify: FDSTOREREMOVE=1\nFDNAME=invalid", + }) + c.Check(s.closedFds, DeepEquals, []int{4}) + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 6, 7}) +} + +func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { + s.fakeEnv["LISTEN_PID"] = "1999" // not 1984 + s.fakeEnv["LISTEN_FDS"] = "3" + s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:memfd-secret-state:snapd.socket" + + // PID mismatch ignores passed fds + c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + + // passed environment variables are cleared + c.Assert(s.fakeEnv, HasLen, 0) +} + +func (s *fdstoreTestSuite) TestInitNoFds(c *C) { + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) +} + +func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { + // two fds, three fd-names + s.fakeEnv["LISTEN_FDS"] = "2" + s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:other.socket:memfd-secret-state" + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) +} + +func (s *fdstoreTestSuite) TestInitPruneMoreThanOneFdOnCloseError(c *C) { + s.fakeEnv["LISTEN_FDS"] = "3" + s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:memfd-secret-state:memfd-secret-state" + + // erroring on the last entry, will make subsequent calls + s.errOn = []string{"close-fd: 4"} + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + + // remove from systemd fdstore as part of the cleanup + c.Check(s.sdNotifyCalls, DeepEquals, []string{ + "sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state", + }) + // only fd (5) because fd (4) should have errored on close + c.Check(s.closedFds, DeepEquals, []int{5}) + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5}) +} + +func (s *fdstoreTestSuite) TestAdd(c *C) { + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 7) + + // but only once + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + // also, cannot add unknown fds + c.Check(fdstore.Add(fdstore.FdName("unknown"), 9), ErrorMatches, `cannot add file descriptor to fdstore: unknown file descriptor name "unknown"`) + // also, cannot add socket fds + c.Check(fdstore.Add(fdstore.FdName("snapd.socket"), 10), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") + c.Check(fdstore.Add(fdstore.FdName("some-svc.socket"), 10), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") + + c.Check(s.sdNotifyCalls, DeepEquals, []string{ + "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]", + }) +} + +func (s *fdstoreTestSuite) TestAddExistingFdError(c *C) { + s.fakeEnv["LISTEN_FDS"] = "1" + s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state" + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + + c.Check(s.sdNotifyCalls, HasLen, 0) +} + +func (s *fdstoreTestSuite) TestAddSdNotifyError(c *C) { + s.errOn = []string{"sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]"} + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), IsNil) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 8) +} + +func (s *fdstoreTestSuite) TestAddLowSystemdVersionError(c *C) { + restore := systemd.MockSystemdVersion(235, nil) + defer restore() + + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: systemd version 235 is too old \(expected at least 236\)`) + + c.Check(s.sdNotifyCalls, HasLen, 0) + c.Check(s.closedFds, HasLen, 0) +} + +func (s *fdstoreTestSuite) TestRemove(c *C) { + s.fakeEnv["LISTEN_FDS"] = "3" + s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket:snapd.socket" + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), IsNil) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 7) + + // cannot remove socket fds + c.Check(fdstore.Remove(fdstore.FdName("snapd.socket")), ErrorMatches, "cannot remove file descriptor from fdstore: sockets cannot be removed") + + c.Check(s.sdNotifyCalls, DeepEquals, []string{ + "sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state", + "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]", + }) + c.Check(s.closedFds, DeepEquals, []int{3}) + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5}) +} + +func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { + s.fakeEnv["LISTEN_FDS"] = "2" + s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket" + + s.errOn = []string{"sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state"} + + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, "boom!") + c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + + c.Check(s.sdNotifyCalls, HasLen, 0) + c.Check(s.closedFds, HasLen, 0) +} + +func (s *fdstoreTestSuite) TestRemoveLowSystemdVersionError(c *C) { + s.fakeEnv["LISTEN_FDS"] = "2" + s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket" + + restore := systemd.MockSystemdVersion(235, nil) + defer restore() + + c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, `cannot remove file descriptor from fdstore: systemd version 235 is too old \(expected at least 236\)`) + + c.Check(s.sdNotifyCalls, HasLen, 0) + c.Check(s.closedFds, HasLen, 0) + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4}) +} + +func (s *fdstoreTestSuite) TestActivationSocketFiles(c *C) { + s.fakeEnv["LISTEN_FDS"] = "4" + s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:snapd.session-agent.socket:memfd-secret-state:snapd.socket" + // fds starts from 3 + socketFds := fdstore.ActivationSocketFds() + c.Check(socketFds, DeepEquals, map[string][]int{ + "snapd.socket": {3, 6}, + "snapd.session-agent.socket": {4}, + }) +} + +func (s *fdstoreTestSuite) TestActivationSocketFilesMissingFdNamesEnv(c *C) { + s.fakeEnv["LISTEN_FDS"] = "4" + // make sure that older versions of systemd (e.g. v219 on amazon-linux-2) + // are supported where the $LISTEN_FDNAMES env var is not passed. + socketFds := fdstore.ActivationSocketFds() + c.Check(socketFds, DeepEquals, map[string][]int{ + "activation-fd-0.socket": {3}, + "activation-fd-1.socket": {4}, + "activation-fd-2.socket": {5}, + "activation-fd-3.socket": {6}, + }) +} + +func (s *fdstoreTestSuite) TestKnownFdNames(c *C) { + c.Assert(fdstore.KnownFdNames(), DeepEquals, map[fdstore.FdName]bool{ + fdstore.FdName("memfd-secret-state"): true, + }) +} From d8d83fb1ff6105f86fa5ac2f60f04d3e008afd25 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Fri, 20 Mar 2026 17:07:44 +0200 Subject: [PATCH 02/10] systemd/fdstore: address review comments Signed-off-by: Zeyad Gouda --- systemd/fdstore/fdstore.go | 50 ++++++++++++++------- systemd/fdstore/fdstore_test.go | 79 ++++++++++++++++++++++++--------- systemd/sdnotify_linux.go | 2 + 3 files changed, 95 insertions(+), 36 deletions(-) diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index a64c7c2a413..afad56db75a 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -1,7 +1,7 @@ // -*- Mode: Go; indent-tabs-mode: t -*- /* - * Copyright (C) 2025 Canonical Ltd + * Copyright (C) 2025-2026 Canonical Ltd * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 3 as @@ -40,6 +40,7 @@ const ( FdNameMemfdSecretState FdName = "memfd-secret-state" ) +// All names that are not sockets that are maintained within snapd. var knownFdNames = map[FdName]bool{ FdNameMemfdSecretState: true, } @@ -67,6 +68,7 @@ var ( ) var fdstore map[FdName][]int +var consumed map[FdName]bool var mu sync.RWMutex func initFdstore() { @@ -88,6 +90,7 @@ func initFdstore() { // Initialize fdstore before any processing so // it is only done once. fdstore = make(map[FdName][]int) + consumed = make(map[FdName]bool) pid, err := strconv.Atoi(osGetenv("LISTEN_PID")) if err != nil || pid != osGetpid() { @@ -142,8 +145,8 @@ func initFdstore() { } if shouldRemove { logger.Noticef("removing unexpected fdstore entry %q", name) - if err := removeUnlocked(name); err != nil { - logger.Noticef("internal error: cannot remove fdstore entry %q: %v\n", name, err) + if err := remove(name); err != nil { + logger.Noticef("internal error: cannot remove fdstore entry %q: %v", name, err) continue } } @@ -165,10 +168,13 @@ func Remove(name FdName) (err error) { mu.Lock() defer mu.Unlock() - return removeUnlocked(name) + return remove(name) } -func removeUnlocked(name FdName) (err error) { +// remove file descriptors from systemd given their name. +// +// Caller must hold the fdstore lock. +func remove(name FdName) (err error) { // FDSTOREREMOVE=1 was added in systemd v236 // // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDSTOREREMOVE=1 @@ -181,6 +187,7 @@ func removeUnlocked(name FdName) (err error) { return err } + // XXX: should consumed fds be closed on removal? var closeErrs []error for _, fd := range fdstore[name] { if err := unixClose(fd); err != nil { @@ -190,31 +197,43 @@ func removeUnlocked(name FdName) (err error) { } delete(fdstore, name) + delete(consumed, name) return strutil.JoinErrors(closeErrs...) } -// Get retrieves file descriptors passed from systemd by their name. -// close-on-exec is set on the returned file descriptor. -1 is returned -// if no matching file descriptor is found or if the passed name -// corresponds to a socket (i.e. ends in ".socket"). To get activation -// sockets use fdstore.ActivationSocketFds() instead. -func Get(name FdName) (fd int) { +// Get retrieves file descriptor passed from systemd by their name. +// close-on-exec is set on the returned file descriptor. An error is +// returned if no matching file descriptor is found, if more than one +// matching file descriptors are found or if the passed name corresponds +// to a socket (i.e. ends in ".socket"). To get activation sockets use +// fdstore.ActivationSocketFds() instead. +func Get(name FdName) (fd int, err error) { initFdstore() mu.RLock() defer mu.RUnlock() + errPrefix := fmt.Sprintf("cannot get file descriptor named %q", name) + + if consumed[name] { + return -1, fmt.Errorf("%s: file descriptor already consumed", errPrefix) + } + if name.isSocket() { // Activation socket file descriptors should be accessed // through ActivationSocketFds. - return -1 + return -1, fmt.Errorf("%s: socket found, use ActivationSocketFds instead", errPrefix) } fds := fdstore[name] if len(fds) != 1 { - return -1 + return -1, fmt.Errorf("%s: no matching file descriptor found", errPrefix) + } else if len(fds) > 1 { + return -1, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) } - return fds[0] + + consumed[name] = true + return fds[0], nil } // Add passes a file descriptor to systemd associated with a name @@ -274,8 +293,9 @@ func ActivationSocketFds() (socketFds map[string][]int) { // // `FileDescriptorName=` was added in systemd version 227. for name, fds := range fdstore { - if name.isSocket() { + if name.isSocket() && !consumed[name] { socketFds[string(name)] = append(socketFds[string(name)], fds...) + consumed[name] = true } } diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index bff54e2a820..1dbd4c331f9 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -1,7 +1,7 @@ // -*- Mode: Go; indent-tabs-mode: t -*- /* - * Copyright (C) 2025 Canonical Ltd + * Copyright (C) 2025-2026 Canonical Ltd * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 3 as @@ -103,15 +103,22 @@ func (s *fdstoreTestSuite) TestGet(c *C) { s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:invalid:snapd.socket:memfd-secret-state:snapd.socket" // fds starts from 3 - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 6) + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 6) // fdstore is lazily initialized once, and clears passed environment c.Assert(s.fakeEnv, HasLen, 0) // more checks - c.Check(fdstore.Get("no-fd"), Equals, -1) // doesn't exist - c.Check(fdstore.Get("invalid"), Equals, -1) // should have been pruned by initialization - c.Check(fdstore.Get("snapd.socket"), Equals, -1) // sockets are not returned + _, err = fdstore.Get("no-fd") // doesn't exist + c.Assert(err, ErrorMatches, `cannot get file descriptor named "no-fd": no matching file descriptor found`) + _, err = fdstore.Get("invalid") // should have been pruned by initialization + c.Assert(err, ErrorMatches, `cannot get file descriptor named "invalid": no matching file descriptor found`) + _, err = fdstore.Get("snapd.socket") // sockets are not returned + c.Assert(err, ErrorMatches, `cannot get file descriptor named "snapd.socket": socket found, use ActivationSocketFds instead`) + _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor already consumed`) // check remove call for "invalid" fd c.Check(s.sdNotifyCalls, DeepEquals, []string{ @@ -128,14 +135,16 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { // PID mismatch ignores passed fds c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) // passed environment variables are cleared c.Assert(s.fakeEnv, HasLen, 0) } func (s *fdstoreTestSuite) TestInitNoFds(c *C) { - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) } @@ -144,7 +153,8 @@ func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { s.fakeEnv["LISTEN_FDS"] = "2" s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:other.socket:memfd-secret-state" - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) } @@ -155,7 +165,8 @@ func (s *fdstoreTestSuite) TestInitPruneMoreThanOneFdOnCloseError(c *C) { // erroring on the last entry, will make subsequent calls s.errOn = []string{"close-fd: 4"} - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) // remove from systemd fdstore as part of the cleanup c.Check(s.sdNotifyCalls, DeepEquals, []string{ @@ -167,9 +178,14 @@ func (s *fdstoreTestSuite) TestInitPruneMoreThanOneFdOnCloseError(c *C) { } func (s *fdstoreTestSuite) TestAdd(c *C) { - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 7) + + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 7) // but only once c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) @@ -188,9 +204,11 @@ func (s *fdstoreTestSuite) TestAddExistingFdError(c *C) { s.fakeEnv["LISTEN_FDS"] = "1" s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state" - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 3) c.Check(s.sdNotifyCalls, HasLen, 0) } @@ -198,12 +216,18 @@ func (s *fdstoreTestSuite) TestAddExistingFdError(c *C) { func (s *fdstoreTestSuite) TestAddSdNotifyError(c *C) { s.errOn = []string{"sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]"} - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, -1) + + _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), IsNil) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 8) + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 8) } func (s *fdstoreTestSuite) TestAddLowSystemdVersionError(c *C) { @@ -220,12 +244,19 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { s.fakeEnv["LISTEN_FDS"] = "3" s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket:snapd.socket" - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 3) + c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), IsNil) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 7) + + fd, err = fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 7) // cannot remove socket fds c.Check(fdstore.Remove(fdstore.FdName("snapd.socket")), ErrorMatches, "cannot remove file descriptor from fdstore: sockets cannot be removed") @@ -244,9 +275,11 @@ func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { s.errOn = []string{"sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state"} - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, "boom!") - c.Check(fdstore.Get(fdstore.FdNameMemfdSecretState), Equals, 3) + + fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(fd, Equals, 3) c.Check(s.sdNotifyCalls, HasLen, 0) c.Check(s.closedFds, HasLen, 0) @@ -275,6 +308,10 @@ func (s *fdstoreTestSuite) TestActivationSocketFiles(c *C) { "snapd.socket": {3, 6}, "snapd.session-agent.socket": {4}, }) + + // now should be consumed + socketFds = fdstore.ActivationSocketFds() + c.Check(socketFds, DeepEquals, map[string][]int{}) } func (s *fdstoreTestSuite) TestActivationSocketFilesMissingFdNamesEnv(c *C) { diff --git a/systemd/sdnotify_linux.go b/systemd/sdnotify_linux.go index 3f99240730f..14b8d465eed 100644 --- a/systemd/sdnotify_linux.go +++ b/systemd/sdnotify_linux.go @@ -42,6 +42,7 @@ func SdNotify(notifyState string) error { if err != nil { return err } + // TODO: keep it open to avoid re-opening and make sure to have O_CLOEXEC defer conn.Close() _, err = conn.Write([]byte(notifyState)) @@ -67,6 +68,7 @@ func SdNotifyWithFds(notifyState string, files ...*os.File) error { if err != nil { return err } + // TODO: keep it open to avoid re-opening and make sure to have O_CLOEXEC defer conn.Close() rawConn, err := conn.SyscallConn() From f3f4c4084703d9b154cdcdbb8ddb35bfad0e4f39 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Fri, 20 Mar 2026 17:13:45 +0200 Subject: [PATCH 03/10] systemd/fdstore: add comment Signed-off-by: Zeyad Gouda --- systemd/fdstore/fdstore.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index afad56db75a..706d3b5122a 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -268,6 +268,8 @@ func Add(name FdName, fd int) error { return fmt.Errorf("cannot add file descriptor to fdstore: %q already exists", name) } + // XXX: set O_CLOEXEC on added fd? + state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) if err := sdNotifyWithFds(state, fd); err != nil { return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) From c3febc74d6a5d97bc9721e357c34ad7afdc2d650 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Fri, 27 Mar 2026 15:25:54 +0200 Subject: [PATCH 04/10] systemd/fdstore: address review comments Signed-off-by: Zeyad Gouda --- netutil/activation.go | 24 ++-- systemd/fdstore/export_test.go | 15 ++- systemd/fdstore/fdstore.go | 112 ++++++++------- systemd/fdstore/fdstore_test.go | 232 ++++++++++++++++++++------------ 4 files changed, 232 insertions(+), 151 deletions(-) diff --git a/netutil/activation.go b/netutil/activation.go index 19f76c2d483..69138a25244 100644 --- a/netutil/activation.go +++ b/netutil/activation.go @@ -72,20 +72,16 @@ func GetListener(socketPath string, listenerMap map[string]net.Listener) (net.Li // ActivationListeners builds a map of addresses to listeners that were passed // during systemd activation -func ActivationListeners() (lns map[string]net.Listener, err error) { - socketFds := fdstore.ActivationSocketFds() +func ActivationListeners() (listenerByAddr map[string]net.Listener, err error) { + listeners, err := fdstore.ActivationListeners() + if err != nil { + return nil, err + } - lns = make(map[string]net.Listener, len(socketFds)) - for name, fds := range socketFds { - for _, fd := range fds { - f := os.NewFile(uintptr(fd), name) - ln, err := net.FileListener(f) - if err != nil { - return nil, err - } - addr := ln.Addr().String() - lns[addr] = ln - } + listenerByAddr = make(map[string]net.Listener, len(listeners)) + for _, listener := range listeners { + addr := listener.Addr().String() + listenerByAddr[addr] = listener } - return lns, nil + return listenerByAddr, nil } diff --git a/systemd/fdstore/export_test.go b/systemd/fdstore/export_test.go index 4f3a71bde3a..9e478c4897a 100644 --- a/systemd/fdstore/export_test.go +++ b/systemd/fdstore/export_test.go @@ -20,6 +20,9 @@ package fdstore import ( + "net" + "os" + "github.com/snapcore/snapd/testutil" ) @@ -39,14 +42,14 @@ func MockOsGetpid(f func() int) (restore func()) { return testutil.Mock(&osGetpid, f) } -func MockUnixClose(f func(fd int) (err error)) (restore func()) { - return testutil.Mock(&unixClose, f) -} - func MockUnixCloseOnExec(f func(fd int)) (restore func()) { return testutil.Mock(&unixCloseOnExec, f) } +func MockUnixDup(f func(oldfd int) (fd int, err error)) (restore func()) { + return testutil.Mock(&unixDup, f) +} + func MockSdNotify(f func(notifyState string) error) (restore func()) { return testutil.Mock(&sdNotify, f) } @@ -55,6 +58,10 @@ func MockSdNotifyWithFds(f func(notifyState string, fds ...int) error) (restore return testutil.Mock(&sdNotifyWithFds, f) } +func MockNetFileListener(f func(f *os.File) (ln net.Listener, err error)) (restore func()) { + return testutil.Mock(&netFileListener, f) +} + func KnownFdNames() map[FdName]bool { return knownFdNames } diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index 706d3b5122a..875a8575ada 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -21,13 +21,13 @@ package fdstore import ( "fmt" + "net" "os" "strconv" "strings" "sync" "github.com/snapcore/snapd/logger" - "github.com/snapcore/snapd/strutil" "github.com/snapcore/snapd/systemd" "golang.org/x/sys/unix" ) @@ -61,14 +61,17 @@ var ( osUnsetenv = os.Unsetenv osLookupEnv = os.LookupEnv osGetpid = os.Getpid - unixClose = unix.Close unixCloseOnExec = unix.CloseOnExec + unixDup = unix.Dup sdNotify = systemd.SdNotify sdNotifyWithFds = systemd.SdNotifyWithFds + netFileListener = net.FileListener ) -var fdstore map[FdName][]int -var consumed map[FdName]bool +// Note: os.File is used to wrap raw fds so that the +// underlying fds are impicitly closed by finalizer +// for os.File, so no need for extra tracking. +var fdstore map[FdName][]*os.File var mu sync.RWMutex func initFdstore() { @@ -89,8 +92,7 @@ func initFdstore() { // Initialize fdstore before any processing so // it is only done once. - fdstore = make(map[FdName][]int) - consumed = make(map[FdName]bool) + fdstore = make(map[FdName][]*os.File) pid, err := strconv.Atoi(osGetenv("LISTEN_PID")) if err != nil || pid != osGetpid() { @@ -112,7 +114,7 @@ func initFdstore() { names = make([]string, nfds) for i := 0; i < nfds; i++ { // A generic name with .socket suffix is enough - // to be picked up by ActivationSocketFds. + // to be picked up by ActivationListeners. names[i] = fmt.Sprintf("activation-fd-%d.socket", i) } } @@ -125,7 +127,7 @@ func initFdstore() { for i := 0; i < nfds; i++ { fd := sd_LISTEN_FDS_START + i name := FdName(names[i]) - fdstore[name] = append(fdstore[name], fd) + fdstore[name] = append(fdstore[name], os.NewFile(uintptr(fd), string(name))) // TODO: Use raw fcntl and check for errors. unixCloseOnExec(fd) @@ -187,27 +189,22 @@ func remove(name FdName) (err error) { return err } - // XXX: should consumed fds be closed on removal? - var closeErrs []error - for _, fd := range fdstore[name] { - if err := unixClose(fd); err != nil { - // record error and keep going - closeErrs = append(closeErrs, err) - } - } - + // Note: Removing the all references of os.File will impicitly + // close opened fds by finalizer for os.File so no need to + // explicitly call close. delete(fdstore, name) - delete(consumed, name) - return strutil.JoinErrors(closeErrs...) + return nil } -// Get retrieves file descriptor passed from systemd by their name. +// Get retrieves file descriptor passed from systemd by its name. // close-on-exec is set on the returned file descriptor. An error is // returned if no matching file descriptor is found, if more than one // matching file descriptors are found or if the passed name corresponds // to a socket (i.e. ends in ".socket"). To get activation sockets use -// fdstore.ActivationSocketFds() instead. -func Get(name FdName) (fd int, err error) { +// fdstore.ActivationListeners() instead. +// +// It is the caller's responsibility to close f when finished. +func Get(name FdName) (f *os.File, retErr error) { initFdstore() mu.RLock() @@ -215,25 +212,31 @@ func Get(name FdName) (fd int, err error) { errPrefix := fmt.Sprintf("cannot get file descriptor named %q", name) - if consumed[name] { - return -1, fmt.Errorf("%s: file descriptor already consumed", errPrefix) - } - if name.isSocket() { // Activation socket file descriptors should be accessed - // through ActivationSocketFds. - return -1, fmt.Errorf("%s: socket found, use ActivationSocketFds instead", errPrefix) + // through ActivationListeners. + return nil, fmt.Errorf("internal error: %s: socket found, use ActivationListeners instead", errPrefix) } fds := fdstore[name] if len(fds) != 1 { - return -1, fmt.Errorf("%s: no matching file descriptor found", errPrefix) + return nil, fmt.Errorf("%s: no matching file descriptor found", errPrefix) } else if len(fds) > 1 { - return -1, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) + return nil, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) } - consumed[name] = true - return fds[0], nil + duplicatedFd, err := unixDup(int(fds[0].Fd())) + if err != nil { + return nil, err + } + unixCloseOnExec(duplicatedFd) + // Currently no errors are returned below, but wrapping fd + // with os.File is a safety measure in case some error is + // returned below in the future so the finalizer would + // close the duplicated fd implicitly. + f = os.NewFile(uintptr(duplicatedFd), string(name)) + + return f, nil } // Add passes a file descriptor to systemd associated with a name @@ -241,7 +244,9 @@ func Get(name FdName) (fd int, err error) { // // - The file descriptors can be retrieved by calling Get(). // - Only a single file descriptor can associated with a FdName. -func Add(name FdName, fd int) error { +// +// It is the caller's responsibility to close f when finished. +func Add(name FdName, f *os.File) (retErr error) { initFdstore() // FDNAME=... was added in systemd v233, but for the sake @@ -268,38 +273,55 @@ func Add(name FdName, fd int) error { return fmt.Errorf("cannot add file descriptor to fdstore: %q already exists", name) } - // XXX: set O_CLOEXEC on added fd? + duplicatedFd, err := unixDup(int(f.Fd())) + if err != nil { + return err + } + unixCloseOnExec(duplicatedFd) + // Wrapping fd with os.File so that if some error is + // returned below, the finalizer for os.File would + // close the duplicated fd implicitly. + duplicatedFile := os.NewFile(uintptr(duplicatedFd), string(name)) state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) - if err := sdNotifyWithFds(state, fd); err != nil { + if err := sdNotifyWithFds(state, duplicatedFd); err != nil { return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) } - fdstore[name] = []int{fd} + fdstore[name] = []*os.File{duplicatedFile} return nil } -// ActivationSocketFds returns activation socket file descriptors -// that were passed from systemd. Only sockets whose name has a -// ".socket" suffix are returned. -func ActivationSocketFds() (socketFds map[string][]int) { +// ActivationListeners returns activation listeners that were passed +// from systemd. Only sockets whose name has a ".socket" suffix are +// returned. +// +// It is the caller's responsibility to close returned listeners when finished. +func ActivationListeners() (listeners []net.Listener, retErr error) { initFdstore() mu.RLock() defer mu.RUnlock() - socketFds = make(map[string][]int) // The file descriptor name defaults to the name of the socket // unit (including its .socket suffix), unless it was explicitly // assigned by setting `FileDescriptorName=` on the socket unit. // // `FileDescriptorName=` was added in systemd version 227. for name, fds := range fdstore { - if name.isSocket() && !consumed[name] { - socketFds[string(name)] = append(socketFds[string(name)], fds...) - consumed[name] = true + if name.isSocket() { + for _, fd := range fds { + // net.FileListener duplicates the underlying fd, so the + // internally tracked fd is safe even if caller closed + // the listener. + listener, err := netFileListener(fd) + if err != nil { + return nil, err + } + listeners = append(listeners, listener) + } } } - return socketFds + return listeners, nil } diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index 1dbd4c331f9..13bd51e0692 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -22,6 +22,9 @@ package fdstore_test import ( "errors" "fmt" + "net" + "os" + "sort" "testing" . "gopkg.in/check.v1" @@ -41,8 +44,9 @@ type fdstoreTestSuite struct { fakeEnv map[string]string sdNotifyCalls []string errOn []string - closedFds []int closeOnExecFds []int + lastDupFd int + duplicatedFds []int } var _ = Suite(&fdstoreTestSuite{}) @@ -51,8 +55,9 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.fakeEnv = map[string]string{"LISTEN_PID": "1984"} s.sdNotifyCalls = nil s.errOn = nil - s.closedFds = nil s.closeOnExecFds = nil + s.lastDupFd = 1000 + s.duplicatedFds = nil s.AddCleanup(fdstore.MockOsGetenv(func(key string) string { return s.fakeEnv[key] @@ -84,16 +89,14 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.sdNotifyCalls = append(s.sdNotifyCalls, call) return nil })) - s.AddCleanup(fdstore.MockUnixClose(func(fd int) (err error) { - if strutil.ListContains(s.errOn, fmt.Sprintf("close-fd: %d", fd)) { - return errors.New("boom!") - } - s.closedFds = append(s.closedFds, fd) - return nil - })) s.AddCleanup(fdstore.MockUnixCloseOnExec(func(fd int) { s.closeOnExecFds = append(s.closeOnExecFds, fd) })) + s.AddCleanup(fdstore.MockUnixDup(func(oldfd int) (fd int, err error) { + s.duplicatedFds = append(s.duplicatedFds, oldfd) + s.lastDupFd++ + return s.lastDupFd, nil + })) s.AddCleanup(systemd.MockSystemdVersion(236, nil)) s.AddCleanup(fdstore.Clear) } @@ -103,29 +106,40 @@ func (s *fdstoreTestSuite) TestGet(c *C) { s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:invalid:snapd.socket:memfd-secret-state:snapd.socket" // fds starts from 3 - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + s.lastDupFd = 1998 + + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 6) + c.Check(file.Fd(), Equals, uintptr(1999)) + c.Check(file.Name(), Equals, "memfd-secret-state") + c.Check(s.duplicatedFds, DeepEquals, []int{6}) // fdstore is lazily initialized once, and clears passed environment c.Assert(s.fakeEnv, HasLen, 0) // more checks - _, err = fdstore.Get("no-fd") // doesn't exist + file, err = fdstore.Get("no-fd") // doesn't exist c.Assert(err, ErrorMatches, `cannot get file descriptor named "no-fd": no matching file descriptor found`) - _, err = fdstore.Get("invalid") // should have been pruned by initialization + c.Check(file, IsNil) + file, err = fdstore.Get("invalid") // should have been pruned by initialization + c.Check(file, IsNil) c.Assert(err, ErrorMatches, `cannot get file descriptor named "invalid": no matching file descriptor found`) - _, err = fdstore.Get("snapd.socket") // sockets are not returned - c.Assert(err, ErrorMatches, `cannot get file descriptor named "snapd.socket": socket found, use ActivationSocketFds instead`) - _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor already consumed`) + file, err = fdstore.Get("snapd.socket") // sockets are not returned + c.Assert(err, ErrorMatches, `internal error: cannot get file descriptor named "snapd.socket": socket found, use ActivationListeners instead`) + c.Check(file, IsNil) + + file, err = fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, IsNil) + c.Check(file.Fd(), Equals, uintptr(2000)) + c.Check(file.Name(), Equals, "memfd-secret-state") + c.Check(s.duplicatedFds, DeepEquals, []int{6, 6}) // check remove call for "invalid" fd c.Check(s.sdNotifyCalls, DeepEquals, []string{ "sd-notify: FDSTOREREMOVE=1\nFDNAME=invalid", }) - c.Check(s.closedFds, DeepEquals, []int{4}) - c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 6, 7}) + // 1999 and 2000 from dupicated fds on Get + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 6, 7, 1999, 2000}) } func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { @@ -134,8 +148,10 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:memfd-secret-state:snapd.socket" // PID mismatch ignores passed fds - c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) - _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + listeners, err := fdstore.ActivationListeners() + c.Check(err, IsNil) + c.Check(listeners, IsNil) + _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) // passed environment variables are cleared @@ -145,7 +161,9 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { func (s *fdstoreTestSuite) TestInitNoFds(c *C) { _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) + listeners, err := fdstore.ActivationListeners() + c.Check(err, IsNil) + c.Check(listeners, IsNil) } func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { @@ -155,48 +173,36 @@ func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - c.Check(fdstore.ActivationSocketFds(), DeepEquals, map[string][]int{}) -} - -func (s *fdstoreTestSuite) TestInitPruneMoreThanOneFdOnCloseError(c *C) { - s.fakeEnv["LISTEN_FDS"] = "3" - s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:memfd-secret-state:memfd-secret-state" - - // erroring on the last entry, will make subsequent calls - s.errOn = []string{"close-fd: 4"} - - _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - - // remove from systemd fdstore as part of the cleanup - c.Check(s.sdNotifyCalls, DeepEquals, []string{ - "sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state", - }) - // only fd (5) because fd (4) should have errored on close - c.Check(s.closedFds, DeepEquals, []int{5}) - c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5}) + listeners, err := fdstore.ActivationListeners() + c.Check(err, IsNil) + c.Check(listeners, IsNil) } func (s *fdstoreTestSuite) TestAdd(c *C) { + s.lastDupFd = 1973 + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), IsNil) + // 7 is duplicated as 1974 + c.Check(s.duplicatedFds, DeepEquals, []int{7}) - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 7) + c.Check(file.Fd(), Equals, uintptr(1975)) + c.Check(s.duplicatedFds, DeepEquals, []int{7, 1974}) // but only once - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(8, "")), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) // also, cannot add unknown fds - c.Check(fdstore.Add(fdstore.FdName("unknown"), 9), ErrorMatches, `cannot add file descriptor to fdstore: unknown file descriptor name "unknown"`) + c.Check(fdstore.Add(fdstore.FdName("unknown"), os.NewFile(9, "")), ErrorMatches, `cannot add file descriptor to fdstore: unknown file descriptor name "unknown"`) // also, cannot add socket fds - c.Check(fdstore.Add(fdstore.FdName("snapd.socket"), 10), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") - c.Check(fdstore.Add(fdstore.FdName("some-svc.socket"), 10), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") + c.Check(fdstore.Add(fdstore.FdName("snapd.socket"), os.NewFile(10, "")), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") + c.Check(fdstore.Add(fdstore.FdName("some-svc.socket"), os.NewFile(10, "")), ErrorMatches, "cannot add file descriptor to fdstore: sockets are not allowed") c.Check(s.sdNotifyCalls, DeepEquals, []string{ - "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]", + "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [1974]", }) } @@ -204,85 +210,101 @@ func (s *fdstoreTestSuite) TestAddExistingFdError(c *C) { s.fakeEnv["LISTEN_FDS"] = "1" s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state" - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + s.lastDupFd = 1999 + + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 3) + c.Check(file.Fd(), Equals, uintptr(2000)) + c.Check(s.duplicatedFds, DeepEquals, []int{3}) c.Check(s.sdNotifyCalls, HasLen, 0) } func (s *fdstoreTestSuite) TestAddSdNotifyError(c *C) { - s.errOn = []string{"sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]"} + s.lastDupFd = 2026 + + s.errOn = []string{"sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [2027]"} _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) + // duplicated (as 2027) before sd-notify error + c.Check(s.duplicatedFds, DeepEquals, []int{7}) _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 8), IsNil) - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + // 8 is duplicated as 2028 + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(8, "")), IsNil) + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 8) + c.Check(file.Fd(), Equals, uintptr(2029)) // 2029 is duplicated from 2028 (which is duplicate from 8) + c.Check(s.duplicatedFds, DeepEquals, []int{7, 8, 2028}) } func (s *fdstoreTestSuite) TestAddLowSystemdVersionError(c *C) { restore := systemd.MockSystemdVersion(235, nil) defer restore() - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: systemd version 235 is too old \(expected at least 236\)`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: systemd version 235 is too old \(expected at least 236\)`) c.Check(s.sdNotifyCalls, HasLen, 0) - c.Check(s.closedFds, HasLen, 0) } func (s *fdstoreTestSuite) TestRemove(c *C) { s.fakeEnv["LISTEN_FDS"] = "3" s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket:snapd.socket" - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) + s.lastDupFd = 1000 + + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: "memfd-secret-state" already exists`) - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 3) + c.Check(file.Fd(), Equals, uintptr(1001)) + c.Check(s.duplicatedFds, DeepEquals, []int{3}) c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), IsNil) - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, 7), IsNil) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), IsNil) + // 7 is duplicated as 1002 + c.Check(s.duplicatedFds, DeepEquals, []int{3, 7}) - fd, err = fdstore.Get(fdstore.FdNameMemfdSecretState) + file, err = fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 7) + c.Check(file.Fd(), Equals, uintptr(1003)) + c.Check(s.duplicatedFds, DeepEquals, []int{3, 7, 1002}) // cannot remove socket fds c.Check(fdstore.Remove(fdstore.FdName("snapd.socket")), ErrorMatches, "cannot remove file descriptor from fdstore: sockets cannot be removed") c.Check(s.sdNotifyCalls, DeepEquals, []string{ "sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state", - "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [7]", + "sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [1002]", }) - c.Check(s.closedFds, DeepEquals, []int{3}) - c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5}) + // 1001 and 1003 are duplicated from Get, 1002 is duplicated from Add + c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 1001, 1002, 1003}) } func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { s.fakeEnv["LISTEN_FDS"] = "2" s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket" + s.lastDupFd = 1000 + s.errOn = []string{"sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state"} c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, "boom!") - fd, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + file, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, IsNil) - c.Check(fd, Equals, 3) + c.Check(file.Fd(), Equals, uintptr(1001)) + c.Check(s.duplicatedFds, DeepEquals, []int{3}) c.Check(s.sdNotifyCalls, HasLen, 0) - c.Check(s.closedFds, HasLen, 0) } func (s *fdstoreTestSuite) TestRemoveLowSystemdVersionError(c *C) { @@ -295,36 +317,70 @@ func (s *fdstoreTestSuite) TestRemoveLowSystemdVersionError(c *C) { c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, `cannot remove file descriptor from fdstore: systemd version 235 is too old \(expected at least 236\)`) c.Check(s.sdNotifyCalls, HasLen, 0) - c.Check(s.closedFds, HasLen, 0) c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4}) } -func (s *fdstoreTestSuite) TestActivationSocketFiles(c *C) { +type fakeListener struct { + f *os.File +} + +func (*fakeListener) Accept() (net.Conn, error) { panic("unexpected") } +func (*fakeListener) Close() error { panic("unexpected") } +func (*fakeListener) Addr() net.Addr { panic("unexpected") } +func (l *fakeListener) String() string { return fmt.Sprintf("%s (%d)", l.f.Name(), l.f.Fd()) } + +func (s *fdstoreTestSuite) TestActivationListeners(c *C) { s.fakeEnv["LISTEN_FDS"] = "4" s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:snapd.session-agent.socket:memfd-secret-state:snapd.socket" // fds starts from 3 - socketFds := fdstore.ActivationSocketFds() - c.Check(socketFds, DeepEquals, map[string][]int{ - "snapd.socket": {3, 6}, - "snapd.session-agent.socket": {4}, + + restore := fdstore.MockNetFileListener(func(f *os.File) (ln net.Listener, err error) { + return &fakeListener{f}, nil }) + defer restore() - // now should be consumed - socketFds = fdstore.ActivationSocketFds() - c.Check(socketFds, DeepEquals, map[string][]int{}) + listeners, err := fdstore.ActivationListeners() + c.Assert(err, IsNil) + c.Assert(listeners, HasLen, 3) + sort.Slice(listeners, func(i, j int) bool { + return listeners[i].(*fakeListener).String() < listeners[j].(*fakeListener).String() + }) + c.Check(listeners[0].(*fakeListener).String(), Equals, "snapd.session-agent.socket (4)") + c.Check(listeners[1].(*fakeListener).String(), Equals, "snapd.socket (3)") + c.Check(listeners[2].(*fakeListener).String(), Equals, "snapd.socket (6)") + + // another time + listeners, err = fdstore.ActivationListeners() + c.Assert(err, IsNil) + c.Assert(listeners, HasLen, 3) + sort.Slice(listeners, func(i, j int) bool { + return listeners[i].(*fakeListener).String() < listeners[j].(*fakeListener).String() + }) + c.Check(listeners[0].(*fakeListener).String(), Equals, "snapd.session-agent.socket (4)") + c.Check(listeners[1].(*fakeListener).String(), Equals, "snapd.socket (3)") + c.Check(listeners[2].(*fakeListener).String(), Equals, "snapd.socket (6)") } -func (s *fdstoreTestSuite) TestActivationSocketFilesMissingFdNamesEnv(c *C) { +func (s *fdstoreTestSuite) TestActivationListenersMissingFdNamesEnv(c *C) { s.fakeEnv["LISTEN_FDS"] = "4" + + restore := fdstore.MockNetFileListener(func(f *os.File) (ln net.Listener, err error) { + return &fakeListener{f}, nil + }) + defer restore() + // make sure that older versions of systemd (e.g. v219 on amazon-linux-2) // are supported where the $LISTEN_FDNAMES env var is not passed. - socketFds := fdstore.ActivationSocketFds() - c.Check(socketFds, DeepEquals, map[string][]int{ - "activation-fd-0.socket": {3}, - "activation-fd-1.socket": {4}, - "activation-fd-2.socket": {5}, - "activation-fd-3.socket": {6}, + listeners, err := fdstore.ActivationListeners() + c.Assert(err, IsNil) + c.Assert(listeners, HasLen, 4) + sort.Slice(listeners, func(i, j int) bool { + return listeners[i].(*fakeListener).String() < listeners[j].(*fakeListener).String() }) + c.Check(listeners[0].(*fakeListener).String(), Equals, "activation-fd-0.socket (3)") + c.Check(listeners[1].(*fakeListener).String(), Equals, "activation-fd-1.socket (4)") + c.Check(listeners[2].(*fakeListener).String(), Equals, "activation-fd-2.socket (5)") + c.Check(listeners[3].(*fakeListener).String(), Equals, "activation-fd-3.socket (6)") } func (s *fdstoreTestSuite) TestKnownFdNames(c *C) { From 949d8871008b3b481c3e86d9f3f81a2cdc097e49 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Thu, 30 Apr 2026 10:39:06 +0300 Subject: [PATCH 05/10] systemd/fdstore: address review comments Signed-off-by: Zeyad Gouda --- systemd/fdstore/export_test.go | 6 ++- systemd/fdstore/fdstore.go | 66 ++++++++++++++++++++------------- systemd/fdstore/fdstore_test.go | 49 ++++++++++++++++++------ 3 files changed, 84 insertions(+), 37 deletions(-) diff --git a/systemd/fdstore/export_test.go b/systemd/fdstore/export_test.go index 9e478c4897a..8a6a9ae3251 100644 --- a/systemd/fdstore/export_test.go +++ b/systemd/fdstore/export_test.go @@ -54,7 +54,7 @@ func MockSdNotify(f func(notifyState string) error) (restore func()) { return testutil.Mock(&sdNotify, f) } -func MockSdNotifyWithFds(f func(notifyState string, fds ...int) error) (restore func()) { +func MockSdNotifyWithFds(f func(notifyState string, files ...*os.File) error) (restore func()) { return testutil.Mock(&sdNotifyWithFds, f) } @@ -62,6 +62,10 @@ func MockNetFileListener(f func(f *os.File) (ln net.Listener, err error)) (resto return testutil.Mock(&netFileListener, f) } +func MockOsFileClose(f func(*os.File) error) (restore func()) { + return testutil.Mock(&osFileClose, f) +} + func KnownFdNames() map[FdName]bool { return knownFdNames } diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index 875a8575ada..ef167b1f9f3 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -20,6 +20,7 @@ package fdstore import ( + "errors" "fmt" "net" "os" @@ -61,6 +62,7 @@ var ( osUnsetenv = os.Unsetenv osLookupEnv = os.LookupEnv osGetpid = os.Getpid + osFileClose = (*os.File).Close unixCloseOnExec = unix.CloseOnExec unixDup = unix.Dup sdNotify = systemd.SdNotify @@ -157,11 +159,30 @@ func initFdstore() { return } +var ErrUnsupportedSystemdVersion = errors.New("unsupported systemd version") +var ErrNotFound = errors.New("file descriptor not found") + +func checkSystemdVersion() error { + // FDNAME=... was added in systemd v233, but for the sake + // of being consistent with removal (FDSTOREREMOVE=1 was + // added in systemd v236), require at least systemd v236. + // + // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDNAME=%E2%80%A6 + if err := systemd.EnsureAtLeast(236); err != nil { + return fmt.Errorf("%w: %v", ErrUnsupportedSystemdVersion, err) + } + return nil +} + // Remove removes file descriptors from systemd given their name. // Remove cannot remove activation sockets. func Remove(name FdName) (err error) { initFdstore() + if err := checkSystemdVersion(); err != nil { + return fmt.Errorf("cannot remove file descriptor from fdstore: %w", err) + } + if name.isSocket() { // Activation sockets can only be passed down from systemd // i.e. file descriptors whose name has a ".socket" suffix @@ -177,36 +198,35 @@ func Remove(name FdName) (err error) { // // Caller must hold the fdstore lock. func remove(name FdName) (err error) { - // FDSTOREREMOVE=1 was added in systemd v236 - // - // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDSTOREREMOVE=1 - if err := systemd.EnsureAtLeast(236); err != nil { - return fmt.Errorf("cannot remove file descriptor from fdstore: %v", err) - } - state := fmt.Sprintf("FDSTOREREMOVE=1\nFDNAME=%s", name) if err := sdNotify(state); err != nil { return err } - // Note: Removing the all references of os.File will impicitly - // close opened fds by finalizer for os.File so no need to - // explicitly call close. + for _, f := range fdstore[name] { + osFileClose(f) + } delete(fdstore, name) return nil } -// Get retrieves file descriptor passed from systemd by its name. -// close-on-exec is set on the returned file descriptor. An error is -// returned if no matching file descriptor is found, if more than one +// Get retrieves a duplicate of the file descriptor passed from systemd by +// its name. close-on-exec is set on the returned file descriptor. An error +// is returned if no matching file descriptor is found, if more than one // matching file descriptors are found or if the passed name corresponds // to a socket (i.e. ends in ".socket"). To get activation sockets use // fdstore.ActivationListeners() instead. // -// It is the caller's responsibility to close f when finished. +// The fdstore holds a copy of the file descriptor, the caller needs to +// call Remove() on top of closing all privately held references in order +// to release all resources associated with a given fd. func Get(name FdName) (f *os.File, retErr error) { initFdstore() + if err := checkSystemdVersion(); err != nil { + return nil, fmt.Errorf("cannot get file descriptor from fdstore: %w", err) + } + mu.RLock() defer mu.RUnlock() @@ -219,8 +239,8 @@ func Get(name FdName) (f *os.File, retErr error) { } fds := fdstore[name] - if len(fds) != 1 { - return nil, fmt.Errorf("%s: no matching file descriptor found", errPrefix) + if len(fds) == 0 { + return nil, fmt.Errorf("%s: %w", errPrefix, ErrNotFound) } else if len(fds) > 1 { return nil, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) } @@ -245,17 +265,13 @@ func Get(name FdName) (f *os.File, retErr error) { // - The file descriptors can be retrieved by calling Get(). // - Only a single file descriptor can associated with a FdName. // -// It is the caller's responsibility to close f when finished. +// Maintains a copy of the underlying file descriptor internally. It +// is the caller's responsibility to close f when finished. func Add(name FdName, f *os.File) (retErr error) { initFdstore() - // FDNAME=... was added in systemd v233, but for the sake - // of being consistent with removal (FDSTOREREMOVE=1 was - // added in systemd v236), require at least systemd v236. - // - // https://www.freedesktop.org/software/systemd/man/latest/sd_pid_notify_with_fds.html#FDNAME=%E2%80%A6 - if err := systemd.EnsureAtLeast(236); err != nil { - return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) + if err := checkSystemdVersion(); err != nil { + return fmt.Errorf("cannot add file descriptor to fdstore: %w", err) } mu.Lock() @@ -284,7 +300,7 @@ func Add(name FdName, f *os.File) (retErr error) { duplicatedFile := os.NewFile(uintptr(duplicatedFd), string(name)) state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) - if err := sdNotifyWithFds(state, duplicatedFd); err != nil { + if err := sdNotifyWithFds(state, duplicatedFile); err != nil { return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) } diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index 13bd51e0692..d7d02308f4f 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -45,6 +45,7 @@ type fdstoreTestSuite struct { sdNotifyCalls []string errOn []string closeOnExecFds []int + closeFds []int lastDupFd int duplicatedFds []int } @@ -56,6 +57,7 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.sdNotifyCalls = nil s.errOn = nil s.closeOnExecFds = nil + s.closeFds = nil s.lastDupFd = 1000 s.duplicatedFds = nil @@ -81,7 +83,11 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.sdNotifyCalls = append(s.sdNotifyCalls, call) return nil })) - s.AddCleanup(fdstore.MockSdNotifyWithFds(func(notifyState string, fds ...int) error { + s.AddCleanup(fdstore.MockSdNotifyWithFds(func(notifyState string, files ...*os.File) error { + fds := make([]int, len(files)) + for i := range files { + fds[i] = int(files[i].Fd()) + } call := fmt.Sprintf("sd-notify-with-fds: %s %v", notifyState, fds) if strutil.ListContains(s.errOn, call) { return errors.New("boom!") @@ -97,6 +103,10 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.lastDupFd++ return s.lastDupFd, nil })) + s.AddCleanup(fdstore.MockOsFileClose(func(f *os.File) error { + s.closeFds = append(s.closeFds, int(f.Fd())) + return nil + })) s.AddCleanup(systemd.MockSystemdVersion(236, nil)) s.AddCleanup(fdstore.Clear) } @@ -119,11 +129,12 @@ func (s *fdstoreTestSuite) TestGet(c *C) { // more checks file, err = fdstore.Get("no-fd") // doesn't exist - c.Assert(err, ErrorMatches, `cannot get file descriptor named "no-fd": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "no-fd": file descriptor not found`) + c.Assert(err, testutil.ErrorIs, fdstore.ErrNotFound) c.Check(file, IsNil) file, err = fdstore.Get("invalid") // should have been pruned by initialization c.Check(file, IsNil) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "invalid": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "invalid": file descriptor not found`) file, err = fdstore.Get("snapd.socket") // sockets are not returned c.Assert(err, ErrorMatches, `internal error: cannot get file descriptor named "snapd.socket": socket found, use ActivationListeners instead`) c.Check(file, IsNil) @@ -142,6 +153,15 @@ func (s *fdstoreTestSuite) TestGet(c *C) { c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 6, 7, 1999, 2000}) } +func (s *fdstoreTestSuite) TestGetLowSystemdVersionError(c *C) { + restore := systemd.MockSystemdVersion(235, nil) + defer restore() + + _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot get file descriptor from fdstore: unsupported systemd version: systemd version 235 is too old \(expected at least 236\)`) + c.Assert(err, testutil.ErrorIs, fdstore.ErrUnsupportedSystemdVersion) +} + func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { s.fakeEnv["LISTEN_PID"] = "1999" // not 1984 s.fakeEnv["LISTEN_FDS"] = "3" @@ -152,7 +172,7 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { c.Check(err, IsNil) c.Check(listeners, IsNil) _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) // passed environment variables are cleared c.Assert(s.fakeEnv, HasLen, 0) @@ -160,7 +180,7 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { func (s *fdstoreTestSuite) TestInitNoFds(c *C) { _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) listeners, err := fdstore.ActivationListeners() c.Check(err, IsNil) c.Check(listeners, IsNil) @@ -172,7 +192,7 @@ func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:other.socket:memfd-secret-state" _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) listeners, err := fdstore.ActivationListeners() c.Check(err, IsNil) c.Check(listeners, IsNil) @@ -182,7 +202,7 @@ func (s *fdstoreTestSuite) TestAdd(c *C) { s.lastDupFd = 1973 _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), IsNil) // 7 is duplicated as 1974 @@ -228,14 +248,14 @@ func (s *fdstoreTestSuite) TestAddSdNotifyError(c *C) { s.errOn = []string{"sd-notify-with-fds: FDSTORE=1\nFDNAME=memfd-secret-state [2027]"} _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) // duplicated (as 2027) before sd-notify error c.Check(s.duplicatedFds, DeepEquals, []int{7}) _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) - c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": no matching file descriptor found`) + c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) // 8 is duplicated as 2028 c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(8, "")), IsNil) @@ -249,7 +269,9 @@ func (s *fdstoreTestSuite) TestAddLowSystemdVersionError(c *C) { restore := systemd.MockSystemdVersion(235, nil) defer restore() - c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: systemd version 235 is too old \(expected at least 236\)`) + err := fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")) + c.Assert(err, ErrorMatches, `cannot add file descriptor to fdstore: unsupported systemd version: systemd version 235 is too old \(expected at least 236\)`) + c.Assert(err, testutil.ErrorIs, fdstore.ErrUnsupportedSystemdVersion) c.Check(s.sdNotifyCalls, HasLen, 0) } @@ -267,7 +289,9 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { c.Check(file.Fd(), Equals, uintptr(1001)) c.Check(s.duplicatedFds, DeepEquals, []int{3}) + c.Check(s.closeFds, DeepEquals, []int(nil)) c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), IsNil) + c.Check(s.closeFds, DeepEquals, []int{3}) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), IsNil) // 7 is duplicated as 1002 @@ -287,6 +311,7 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { }) // 1001 and 1003 are duplicated from Get, 1002 is duplicated from Add c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4, 5, 1001, 1002, 1003}) + c.Check(s.closeFds, DeepEquals, []int{3}) } func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { @@ -314,7 +339,9 @@ func (s *fdstoreTestSuite) TestRemoveLowSystemdVersionError(c *C) { restore := systemd.MockSystemdVersion(235, nil) defer restore() - c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, `cannot remove file descriptor from fdstore: systemd version 235 is too old \(expected at least 236\)`) + err := fdstore.Remove(fdstore.FdNameMemfdSecretState) + c.Assert(err, ErrorMatches, `cannot remove file descriptor from fdstore: unsupported systemd version: systemd version 235 is too old \(expected at least 236\)`) + c.Assert(err, testutil.ErrorIs, fdstore.ErrUnsupportedSystemdVersion) c.Check(s.sdNotifyCalls, HasLen, 0) c.Check(s.closeOnExecFds, DeepEquals, []int{3, 4}) From 6c74eb0352e8bbb511087395897cf2832e382cc5 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Thu, 30 Apr 2026 10:52:33 +0300 Subject: [PATCH 06/10] data/systemd: add service unit option to support fdstore Signed-off-by: Zeyad Gouda --- data/systemd/snapd.service.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/data/systemd/snapd.service.in b/data/systemd/snapd.service.in index 8dda3a90ec6..f57ce524d39 100644 --- a/data/systemd/snapd.service.in +++ b/data/systemd/snapd.service.in @@ -28,6 +28,10 @@ SuccessExitStatus=42 RestartPreventExitStatus=42 KillMode=process KeyringMode=shared +FileDescriptorStoreMax=1024 +# Should this be enabled to survive soft reboots and service stop/start +# starting with systmed v254+? +#FileDescriptorStorePreserve=yes [Install] WantedBy=multi-user.target From 76af8a998be0046b264b6a607a33e5b6f49fd2c2 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Wed, 20 May 2026 18:05:09 +0300 Subject: [PATCH 07/10] systemd/fdstore: address review comments Signed-off-by: Zeyad Gouda --- data/systemd/snapd.service.in | 3 -- systemd/fdstore/export_test.go | 12 ------ systemd/fdstore/fdstore.go | 69 +++++++++++++++------------------ systemd/fdstore/fdstore_test.go | 65 +++++++++++++++---------------- 4 files changed, 64 insertions(+), 85 deletions(-) diff --git a/data/systemd/snapd.service.in b/data/systemd/snapd.service.in index f57ce524d39..5fcd5604f96 100644 --- a/data/systemd/snapd.service.in +++ b/data/systemd/snapd.service.in @@ -29,9 +29,6 @@ RestartPreventExitStatus=42 KillMode=process KeyringMode=shared FileDescriptorStoreMax=1024 -# Should this be enabled to survive soft reboots and service stop/start -# starting with systmed v254+? -#FileDescriptorStorePreserve=yes [Install] WantedBy=multi-user.target diff --git a/systemd/fdstore/export_test.go b/systemd/fdstore/export_test.go index 8a6a9ae3251..dcbf60566a8 100644 --- a/systemd/fdstore/export_test.go +++ b/systemd/fdstore/export_test.go @@ -26,18 +26,6 @@ import ( "github.com/snapcore/snapd/testutil" ) -func MockOsGetenv(f func(key string) string) (restore func()) { - return testutil.Mock(&osGetenv, f) -} - -func MockOsUnsetenv(f func(key string) error) (restore func()) { - return testutil.Mock(&osUnsetenv, f) -} - -func MockOsLookupEnv(f func(key string) (string, bool)) (restore func()) { - return testutil.Mock(&osLookupEnv, f) -} - func MockOsGetpid(f func() int) (restore func()) { return testutil.Mock(&osGetpid, f) } diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index ef167b1f9f3..6bcf4201ad9 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -58,9 +58,6 @@ func (name FdName) isSocket() bool { } var ( - osGetenv = os.Getenv - osUnsetenv = os.Unsetenv - osLookupEnv = os.LookupEnv osGetpid = os.Getpid osFileClose = (*os.File).Close unixCloseOnExec = unix.CloseOnExec @@ -87,27 +84,27 @@ func initFdstore() { // Make sure initialization only happens once, only here. defer func() { - osUnsetenv("LISTEN_PID") - osUnsetenv("LISTEN_FDS") - osUnsetenv("LISTEN_FDNAMES") + os.Unsetenv("LISTEN_PID") + os.Unsetenv("LISTEN_FDS") + os.Unsetenv("LISTEN_FDNAMES") }() // Initialize fdstore before any processing so // it is only done once. fdstore = make(map[FdName][]*os.File) - pid, err := strconv.Atoi(osGetenv("LISTEN_PID")) + pid, err := strconv.Atoi(os.Getenv("LISTEN_PID")) if err != nil || pid != osGetpid() { return } - nfds, err := strconv.Atoi(osGetenv("LISTEN_FDS")) + nfds, err := strconv.Atoi(os.Getenv("LISTEN_FDS")) if err != nil || nfds == 0 { return } var names []string - namesEnv, namesEnvExists := osLookupEnv("LISTEN_FDNAMES") + namesEnv, namesEnvExists := os.LookupEnv("LISTEN_FDNAMES") if namesEnvExists { names = strings.Split(namesEnv, ":") } else { @@ -142,7 +139,7 @@ func initFdstore() { logger.Noticef("unexpected fdstore entry %q found: %v", name, err) shouldRemove = true } - // Only activation sockets can be associated with multiple fds. + // We only allow activation sockets to be associated with multiple fds. if !name.isSocket() && len(fds) != 1 { logger.Noticef("unexpected fdstore entry %[1]q found: %[1]q has more than one fd", name) shouldRemove = true @@ -155,8 +152,6 @@ func initFdstore() { } } } - - return } var ErrUnsupportedSystemdVersion = errors.New("unsupported systemd version") @@ -210,12 +205,24 @@ func remove(name FdName) (err error) { return nil } +func duplicateFile(name FdName, f *os.File) (*os.File, error) { + duplicatedFd, err := unixDup(int(f.Fd())) + if err != nil { + return nil, err + } + // TODO: Use raw fcntl and check for errors. + unixCloseOnExec(duplicatedFd) + + // Wrapping fd with os.File is a safety measure so that the finalizer + // would close the duplicated fd implicitly if it goes out of scope. + return os.NewFile(uintptr(duplicatedFd), string(name)), nil +} + // Get retrieves a duplicate of the file descriptor passed from systemd by // its name. close-on-exec is set on the returned file descriptor. An error -// is returned if no matching file descriptor is found, if more than one -// matching file descriptors are found or if the passed name corresponds -// to a socket (i.e. ends in ".socket"). To get activation sockets use -// fdstore.ActivationListeners() instead. +// matching ErrNotFound is returned if no matching file descriptor is found. +// Passed name cannot be a socket (i.e. cannot end in ".socket"), for +// activation sockets use ActivationListeners() instead. // // The fdstore holds a copy of the file descriptor, the caller needs to // call Remove() on top of closing all privately held references in order @@ -227,9 +234,6 @@ func Get(name FdName) (f *os.File, retErr error) { return nil, fmt.Errorf("cannot get file descriptor from fdstore: %w", err) } - mu.RLock() - defer mu.RUnlock() - errPrefix := fmt.Sprintf("cannot get file descriptor named %q", name) if name.isSocket() { @@ -238,23 +242,18 @@ func Get(name FdName) (f *os.File, retErr error) { return nil, fmt.Errorf("internal error: %s: socket found, use ActivationListeners instead", errPrefix) } + mu.RLock() + defer mu.RUnlock() + fds := fdstore[name] if len(fds) == 0 { return nil, fmt.Errorf("%s: %w", errPrefix, ErrNotFound) - } else if len(fds) > 1 { - return nil, fmt.Errorf("%s: found more than one matching file descriptors", errPrefix) } - duplicatedFd, err := unixDup(int(fds[0].Fd())) + f, err := duplicateFile(name, fds[0]) if err != nil { return nil, err } - unixCloseOnExec(duplicatedFd) - // Currently no errors are returned below, but wrapping fd - // with os.File is a safety measure in case some error is - // returned below in the future so the finalizer would - // close the duplicated fd implicitly. - f = os.NewFile(uintptr(duplicatedFd), string(name)) return f, nil } @@ -274,9 +273,6 @@ func Add(name FdName, f *os.File) (retErr error) { return fmt.Errorf("cannot add file descriptor to fdstore: %w", err) } - mu.Lock() - defer mu.Unlock() - if err := name.validate(); err != nil { return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) } @@ -285,19 +281,18 @@ func Add(name FdName, f *os.File) (retErr error) { // i.e. file descriptors whose name has a ".socket" suffix return fmt.Errorf("cannot add file descriptor to fdstore: sockets are not allowed") } + + mu.Lock() + defer mu.Unlock() + if len(fdstore[name]) != 0 { return fmt.Errorf("cannot add file descriptor to fdstore: %q already exists", name) } - duplicatedFd, err := unixDup(int(f.Fd())) + duplicatedFile, err := duplicateFile(name, f) if err != nil { return err } - unixCloseOnExec(duplicatedFd) - // Wrapping fd with os.File so that if some error is - // returned below, the finalizer for os.File would - // close the duplicated fd implicitly. - duplicatedFile := os.NewFile(uintptr(duplicatedFd), string(name)) state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) if err := sdNotifyWithFds(state, duplicatedFile); err != nil { diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index d7d02308f4f..5195c1b6f13 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -41,7 +41,6 @@ func Test(t *testing.T) { TestingT(t) } type fdstoreTestSuite struct { testutil.BaseTest - fakeEnv map[string]string sdNotifyCalls []string errOn []string closeOnExecFds []int @@ -53,7 +52,6 @@ type fdstoreTestSuite struct { var _ = Suite(&fdstoreTestSuite{}) func (s *fdstoreTestSuite) SetUpTest(c *C) { - s.fakeEnv = map[string]string{"LISTEN_PID": "1984"} s.sdNotifyCalls = nil s.errOn = nil s.closeOnExecFds = nil @@ -61,17 +59,14 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { s.lastDupFd = 1000 s.duplicatedFds = nil - s.AddCleanup(fdstore.MockOsGetenv(func(key string) string { - return s.fakeEnv[key] - })) - s.AddCleanup(fdstore.MockOsUnsetenv(func(key string) error { - delete(s.fakeEnv, key) - return nil - })) - s.AddCleanup(fdstore.MockOsLookupEnv(func(key string) (string, bool) { - val, exists := s.fakeEnv[key] - return val, exists - })) + os.Setenv("LISTEN_PID", "1984") + os.Unsetenv("LISTEN_FDS") + os.Unsetenv("LISTEN_FDNAMES") + s.AddCleanup(func() { + os.Unsetenv("LISTEN_PID") + os.Unsetenv("LISTEN_FDS") + os.Unsetenv("LISTEN_FDNAMES") + }) s.AddCleanup(fdstore.MockOsGetpid(func() int { return 1984 })) @@ -112,8 +107,8 @@ func (s *fdstoreTestSuite) SetUpTest(c *C) { } func (s *fdstoreTestSuite) TestGet(c *C) { - s.fakeEnv["LISTEN_FDS"] = "5" - s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:invalid:snapd.socket:memfd-secret-state:snapd.socket" + os.Setenv("LISTEN_FDS", "5") + os.Setenv("LISTEN_FDNAMES", "snapd.socket:invalid:snapd.socket:memfd-secret-state:snapd.socket") // fds starts from 3 s.lastDupFd = 1998 @@ -125,7 +120,9 @@ func (s *fdstoreTestSuite) TestGet(c *C) { c.Check(s.duplicatedFds, DeepEquals, []int{6}) // fdstore is lazily initialized once, and clears passed environment - c.Assert(s.fakeEnv, HasLen, 0) + c.Assert(os.Getenv("LISTEN_PID"), Equals, "") + c.Assert(os.Getenv("LISTEN_FDS"), Equals, "") + c.Assert(os.Getenv("LISTEN_FDNAMES"), Equals, "") // more checks file, err = fdstore.Get("no-fd") // doesn't exist @@ -163,9 +160,9 @@ func (s *fdstoreTestSuite) TestGetLowSystemdVersionError(c *C) { } func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { - s.fakeEnv["LISTEN_PID"] = "1999" // not 1984 - s.fakeEnv["LISTEN_FDS"] = "3" - s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:memfd-secret-state:snapd.socket" + os.Setenv("LISTEN_PID", "1999") // not 1984 + os.Setenv("LISTEN_FDS", "3") + os.Setenv("LISTEN_FDNAMES", "snapd.socket:memfd-secret-state:snapd.socket") // PID mismatch ignores passed fds listeners, err := fdstore.ActivationListeners() @@ -175,7 +172,9 @@ func (s *fdstoreTestSuite) TestInitBadPIDError(c *C) { c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) // passed environment variables are cleared - c.Assert(s.fakeEnv, HasLen, 0) + c.Assert(os.Getenv("LISTEN_PID"), Equals, "") + c.Assert(os.Getenv("LISTEN_FDS"), Equals, "") + c.Assert(os.Getenv("LISTEN_FDNAMES"), Equals, "") } func (s *fdstoreTestSuite) TestInitNoFds(c *C) { @@ -188,8 +187,8 @@ func (s *fdstoreTestSuite) TestInitNoFds(c *C) { func (s *fdstoreTestSuite) TestInitEnvMismatchError(c *C) { // two fds, three fd-names - s.fakeEnv["LISTEN_FDS"] = "2" - s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:other.socket:memfd-secret-state" + os.Setenv("LISTEN_FDS", "2") + os.Setenv("LISTEN_FDNAMES", "snapd.socket:other.socket:memfd-secret-state") _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) @@ -227,8 +226,8 @@ func (s *fdstoreTestSuite) TestAdd(c *C) { } func (s *fdstoreTestSuite) TestAddExistingFdError(c *C) { - s.fakeEnv["LISTEN_FDS"] = "1" - s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state" + os.Setenv("LISTEN_FDS", "1") + os.Setenv("LISTEN_FDNAMES", "memfd-secret-state") s.lastDupFd = 1999 @@ -277,8 +276,8 @@ func (s *fdstoreTestSuite) TestAddLowSystemdVersionError(c *C) { } func (s *fdstoreTestSuite) TestRemove(c *C) { - s.fakeEnv["LISTEN_FDS"] = "3" - s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket:snapd.socket" + os.Setenv("LISTEN_FDS", "3") + os.Setenv("LISTEN_FDNAMES", "memfd-secret-state:snapd.socket:snapd.socket") s.lastDupFd = 1000 @@ -315,8 +314,8 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { } func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { - s.fakeEnv["LISTEN_FDS"] = "2" - s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket" + os.Setenv("LISTEN_FDS", "2") + os.Setenv("LISTEN_FDNAMES", "memfd-secret-state:snapd.socket") s.lastDupFd = 1000 @@ -333,8 +332,8 @@ func (s *fdstoreTestSuite) TestRemoveSdNotifyError(c *C) { } func (s *fdstoreTestSuite) TestRemoveLowSystemdVersionError(c *C) { - s.fakeEnv["LISTEN_FDS"] = "2" - s.fakeEnv["LISTEN_FDNAMES"] = "memfd-secret-state:snapd.socket" + os.Setenv("LISTEN_FDS", "2") + os.Setenv("LISTEN_FDNAMES", "memfd-secret-state:snapd.socket") restore := systemd.MockSystemdVersion(235, nil) defer restore() @@ -357,8 +356,8 @@ func (*fakeListener) Addr() net.Addr { panic("unexpected") } func (l *fakeListener) String() string { return fmt.Sprintf("%s (%d)", l.f.Name(), l.f.Fd()) } func (s *fdstoreTestSuite) TestActivationListeners(c *C) { - s.fakeEnv["LISTEN_FDS"] = "4" - s.fakeEnv["LISTEN_FDNAMES"] = "snapd.socket:snapd.session-agent.socket:memfd-secret-state:snapd.socket" + os.Setenv("LISTEN_FDS", "4") + os.Setenv("LISTEN_FDNAMES", "snapd.socket:snapd.session-agent.socket:memfd-secret-state:snapd.socket") // fds starts from 3 restore := fdstore.MockNetFileListener(func(f *os.File) (ln net.Listener, err error) { @@ -389,7 +388,7 @@ func (s *fdstoreTestSuite) TestActivationListeners(c *C) { } func (s *fdstoreTestSuite) TestActivationListenersMissingFdNamesEnv(c *C) { - s.fakeEnv["LISTEN_FDS"] = "4" + os.Setenv("LISTEN_FDS", "4") restore := fdstore.MockNetFileListener(func(f *os.File) (ln net.Listener, err error) { return &fakeListener{f}, nil From 87d0a60ce65adcd3afae9125e5a823b9af4fac14 Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Mon, 1 Jun 2026 18:13:20 +0300 Subject: [PATCH 08/10] fixup! systemd/fdstore: address review comments --- systemd/fdstore/fdstore.go | 31 +++++++++++++++++-------------- systemd/fdstore/fdstore_test.go | 5 +++++ 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index 6bcf4201ad9..8f8041842e1 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -33,8 +33,7 @@ import ( "golang.org/x/sys/unix" ) -const sd_LISTEN_FDS_START = 3 - +// FdName uniquely identifies file descriptors passed from systemd to snapd. type FdName string const ( @@ -73,6 +72,10 @@ var ( var fdstore map[FdName][]*os.File var mu sync.RWMutex +// sd_LISTEN_FDS_START is the starting file descriptor number for file descriptors +// passed from systemd. +const sd_LISTEN_FDS_START = 3 + func initFdstore() { mu.Lock() defer mu.Unlock() @@ -171,7 +174,7 @@ func checkSystemdVersion() error { // Remove removes file descriptors from systemd given their name. // Remove cannot remove activation sockets. -func Remove(name FdName) (err error) { +func Remove(name FdName) error { initFdstore() if err := checkSystemdVersion(); err != nil { @@ -184,6 +187,10 @@ func Remove(name FdName) (err error) { return fmt.Errorf("cannot remove file descriptor from fdstore: sockets cannot be removed") } + if fdstore[name] == nil { + return fmt.Errorf("cannot remove file descriptor from fdstore: %w", ErrNotFound) + } + mu.Lock() defer mu.Unlock() return remove(name) @@ -192,7 +199,7 @@ func Remove(name FdName) (err error) { // remove file descriptors from systemd given their name. // // Caller must hold the fdstore lock. -func remove(name FdName) (err error) { +func remove(name FdName) error { state := fmt.Sprintf("FDSTOREREMOVE=1\nFDNAME=%s", name) if err := sdNotify(state); err != nil { return err @@ -227,7 +234,7 @@ func duplicateFile(name FdName, f *os.File) (*os.File, error) { // The fdstore holds a copy of the file descriptor, the caller needs to // call Remove() on top of closing all privately held references in order // to release all resources associated with a given fd. -func Get(name FdName) (f *os.File, retErr error) { +func Get(name FdName) (*os.File, error) { initFdstore() if err := checkSystemdVersion(); err != nil { @@ -250,12 +257,7 @@ func Get(name FdName) (f *os.File, retErr error) { return nil, fmt.Errorf("%s: %w", errPrefix, ErrNotFound) } - f, err := duplicateFile(name, fds[0]) - if err != nil { - return nil, err - } - - return f, nil + return duplicateFile(name, fds[0]) } // Add passes a file descriptor to systemd associated with a name @@ -266,7 +268,7 @@ func Get(name FdName) (f *os.File, retErr error) { // // Maintains a copy of the underlying file descriptor internally. It // is the caller's responsibility to close f when finished. -func Add(name FdName, f *os.File) (retErr error) { +func Add(name FdName, f *os.File) error { initFdstore() if err := checkSystemdVersion(); err != nil { @@ -296,6 +298,7 @@ func Add(name FdName, f *os.File) (retErr error) { state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) if err := sdNotifyWithFds(state, duplicatedFile); err != nil { + duplicatedFile.Close() // clean up the duplicated fd return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) } @@ -305,10 +308,10 @@ func Add(name FdName, f *os.File) (retErr error) { // ActivationListeners returns activation listeners that were passed // from systemd. Only sockets whose name has a ".socket" suffix are -// returned. +// returned. Order of returned listeners is not deterministic. // // It is the caller's responsibility to close returned listeners when finished. -func ActivationListeners() (listeners []net.Listener, retErr error) { +func ActivationListeners() (listeners []net.Listener, err error) { initFdstore() mu.RLock() diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index 5195c1b6f13..831460c566c 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -292,6 +292,9 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), IsNil) c.Check(s.closeFds, DeepEquals, []int{3}) + // cannot remove again + c.Check(fdstore.Remove(fdstore.FdNameMemfdSecretState), ErrorMatches, `cannot remove file descriptor from fdstore: file descriptor not found`) + c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), IsNil) // 7 is duplicated as 1002 c.Check(s.duplicatedFds, DeepEquals, []int{3, 7}) @@ -303,6 +306,8 @@ func (s *fdstoreTestSuite) TestRemove(c *C) { // cannot remove socket fds c.Check(fdstore.Remove(fdstore.FdName("snapd.socket")), ErrorMatches, "cannot remove file descriptor from fdstore: sockets cannot be removed") + // or unknown fds + c.Check(fdstore.Remove(fdstore.FdName("unknown")), ErrorMatches, `cannot remove file descriptor from fdstore: file descriptor not found`) c.Check(s.sdNotifyCalls, DeepEquals, []string{ "sd-notify: FDSTOREREMOVE=1\nFDNAME=memfd-secret-state", From b65c1331b892b49f4ec60f8ccd98fbaecaff20ee Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Wed, 3 Jun 2026 10:22:11 +0300 Subject: [PATCH 09/10] fixup! systemd/fdstore: address review comments --- systemd/fdstore/fdstore.go | 21 ++++++++++++++++----- systemd/fdstore/fdstore_test.go | 3 +++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index 8f8041842e1..ba712ca5d40 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -67,7 +67,7 @@ var ( ) // Note: os.File is used to wrap raw fds so that the -// underlying fds are impicitly closed by finalizer +// underlying fds are implicitly closed by finalizer // for os.File, so no need for extra tracking. var fdstore map[FdName][]*os.File var mu sync.RWMutex @@ -187,12 +187,13 @@ func Remove(name FdName) error { return fmt.Errorf("cannot remove file descriptor from fdstore: sockets cannot be removed") } + mu.Lock() + defer mu.Unlock() + if fdstore[name] == nil { return fmt.Errorf("cannot remove file descriptor from fdstore: %w", ErrNotFound) } - mu.Lock() - defer mu.Unlock() return remove(name) } @@ -298,7 +299,7 @@ func Add(name FdName, f *os.File) error { state := fmt.Sprintf("FDSTORE=1\nFDNAME=%s", name) if err := sdNotifyWithFds(state, duplicatedFile); err != nil { - duplicatedFile.Close() // clean up the duplicated fd + osFileClose(duplicatedFile) // clean up the duplicated fd return fmt.Errorf("cannot add file descriptor to fdstore: %v", err) } @@ -311,12 +312,22 @@ func Add(name FdName, f *os.File) error { // returned. Order of returned listeners is not deterministic. // // It is the caller's responsibility to close returned listeners when finished. -func ActivationListeners() (listeners []net.Listener, err error) { +func ActivationListeners() (listeners []net.Listener, retErr error) { initFdstore() mu.RLock() defer mu.RUnlock() + defer func() { + // Clean up collected listeners on error since net.FileListener + // duplicates the underlying fd. + if retErr != nil && len(listeners) > 0 { + for _, l := range listeners { + l.Close() + } + } + }() + // The file descriptor name defaults to the name of the socket // unit (including its .socket suffix), unless it was explicitly // assigned by setting `FileDescriptorName=` on the socket unit. diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index 831460c566c..37a2f11d554 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -249,9 +249,12 @@ func (s *fdstoreTestSuite) TestAddSdNotifyError(c *C) { _, err := fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) + c.Check(s.closeFds, DeepEquals, []int(nil)) c.Check(fdstore.Add(fdstore.FdNameMemfdSecretState, os.NewFile(7, "")), ErrorMatches, `cannot add file descriptor to fdstore: boom!`) // duplicated (as 2027) before sd-notify error c.Check(s.duplicatedFds, DeepEquals, []int{7}) + // duplicated fd should be closed on error + c.Check(s.closeFds, DeepEquals, []int{2027}) _, err = fdstore.Get(fdstore.FdNameMemfdSecretState) c.Assert(err, ErrorMatches, `cannot get file descriptor named "memfd-secret-state": file descriptor not found`) From 5059e41954375a6f0eea3d7db3fc7c1010898c3f Mon Sep 17 00:00:00 2001 From: Zeyad Gouda Date: Fri, 5 Jun 2026 13:13:13 +0300 Subject: [PATCH 10/10] fixup! systemd/fdstore: fix activation listeners cleanup Signed-off-by: Zeyad Gouda --- systemd/fdstore/fdstore.go | 3 ++- systemd/fdstore/fdstore_test.go | 40 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/systemd/fdstore/fdstore.go b/systemd/fdstore/fdstore.go index ba712ca5d40..d46f99db75e 100644 --- a/systemd/fdstore/fdstore.go +++ b/systemd/fdstore/fdstore.go @@ -312,12 +312,13 @@ func Add(name FdName, f *os.File) error { // returned. Order of returned listeners is not deterministic. // // It is the caller's responsibility to close returned listeners when finished. -func ActivationListeners() (listeners []net.Listener, retErr error) { +func ActivationListeners() (retListeners []net.Listener, retErr error) { initFdstore() mu.RLock() defer mu.RUnlock() + var listeners []net.Listener defer func() { // Clean up collected listeners on error since net.FileListener // duplicates the underlying fd. diff --git a/systemd/fdstore/fdstore_test.go b/systemd/fdstore/fdstore_test.go index 37a2f11d554..01a1f2ddd97 100644 --- a/systemd/fdstore/fdstore_test.go +++ b/systemd/fdstore/fdstore_test.go @@ -417,6 +417,46 @@ func (s *fdstoreTestSuite) TestActivationListenersMissingFdNamesEnv(c *C) { c.Check(listeners[3].(*fakeListener).String(), Equals, "activation-fd-3.socket (6)") } +type fakeClosableListener struct { + closed int +} + +func (*fakeClosableListener) Accept() (net.Conn, error) { panic("unexpected") } +func (l *fakeClosableListener) Close() error { + l.closed++ + return nil +} +func (*fakeClosableListener) Addr() net.Addr { panic("unexpected") } + +func (s *fdstoreTestSuite) TestActivationListenersCleansUpCollectedListenersOnError(c *C) { + os.Setenv("LISTEN_FDS", "3") + os.Setenv("LISTEN_FDNAMES", "snapd.socket:memfd-secret-state:snapd.socket") + + created := make([]*fakeClosableListener, 0, 1) + calls := 0 + restore := fdstore.MockNetFileListener(func(f *os.File) (ln net.Listener, err error) { + if f.Name() != "snapd.socket" { + c.Fatalf("unexpected fd: %q", f.Name()) + } + + calls++ + if calls == 1 { + l := &fakeClosableListener{} + created = append(created, l) + return l, nil + } + + return nil, errors.New("boom") + }) + defer restore() + + listeners, err := fdstore.ActivationListeners() + c.Assert(err, ErrorMatches, "boom") + c.Check(listeners, IsNil) + c.Assert(created, HasLen, 1) + c.Check(created[0].closed, Equals, 1) +} + func (s *fdstoreTestSuite) TestKnownFdNames(c *C) { c.Assert(fdstore.KnownFdNames(), DeepEquals, map[fdstore.FdName]bool{ fdstore.FdName("memfd-secret-state"): true,