From 0559a1e0afad8560f6ff01f47593e46302b4f14d Mon Sep 17 00:00:00 2001 From: Fred Lotter Date: Thu, 6 Jul 2023 11:59:47 +0200 Subject: [PATCH] Code Review Changes #1 --- internals/daemon/daemon.go | 60 ++++++++++----------- internals/daemon/daemon_test.go | 96 +++++++++++++++++++++++++-------- 2 files changed, 102 insertions(+), 54 deletions(-) diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go index 7cee9f79f..1e9eb843e 100644 --- a/internals/daemon/daemon.go +++ b/internals/daemon/daemon.go @@ -741,53 +741,47 @@ func commandReboot(rebootDelay time.Duration) error { } var ( - // checkCapSysBoot returns nil if the system has the correct - // permissions to issue a reboot request to the Linux kernel - checkCapSysBoot = func() error { - var caps unix.CapUserData - // We deliberately use v1 caps here due to: - // https://github.com/golang/go/issues/44312 - hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_1} - err := unix.Capget(&hdr, &caps) - if err == nil { - if (int32(caps.Effective) & (1 << unix.CAP_SYS_BOOT)) == 0 { - err = fmt.Errorf("no capability to reboot") - } - } - return err - } - - shutdownSyscall = func() { - // As per the requirements of the reboot syscall, we have to - // first call sync. - unix.Sync() - // This syscall can fail (EINVAL/EPERM) if invalid arguments are - // supplied or CAP_SYS_BOOT capability is missing. We cover the - // latter case in a separate capability check, so this will not - // happen here, but let's panic if we see something to make sure - // we catch anything unexpected. - err := unix.Reboot(unix.LINUX_REBOOT_CMD_RESTART) - if err != nil { - panic("internal error: reboot syscall failed") - } - } + capGetSyscall = unix.Capget + syncSyscall = unix.Sync + rebootSyscall = unix.Reboot ) -// syscallReboot performs a reboot using direct Linux kernel syscalls. +// syscallReboot performs a delayed async reboot using direct Linux +// kernel syscalls. // // Note: Reboot message not currently supported. func syscallReboot(rebootDelay time.Duration) error { - err := checkCapSysBoot() + var caps unix.CapUserData + // We deliberately use v1 caps here due to: + // https://github.com/golang/go/issues/44312 + hdr := unix.CapUserHeader{Version: unix.LINUX_CAPABILITY_VERSION_1} + err := capGetSyscall(&hdr, &caps) if err != nil { return err } + if (int32(caps.Effective) & (1 << unix.CAP_SYS_BOOT)) == 0 { + return fmt.Errorf("no capability to reboot") + } if rebootDelay < 0 { rebootDelay = 0 } // This has to be non-blocking, and scheduled for a future // point in time to mimic shutdown. - time.AfterFunc(rebootDelay, shutdownSyscall) + time.AfterFunc(rebootDelay, func() { + // As per the requirements of the reboot syscall, we have to + // first call sync. + syncSyscall() + // This syscall can fail (EINVAL/EPERM) if invalid arguments are + // supplied or CAP_SYS_BOOT capability is missing. We cover the + // latter case in a separate capability check, so this will not + // happen here, but let's panic if we see something to make sure + // we catch anything unexpected. + err := rebootSyscall(unix.LINUX_REBOOT_CMD_RESTART) + if err != nil { + panic("internal error: reboot syscall failed") + } + }) return nil } diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go index 82a1c75a0..6e8cbe7a6 100644 --- a/internals/daemon/daemon_test.go +++ b/internals/daemon/daemon_test.go @@ -29,6 +29,7 @@ import ( "testing" "time" + "golang.org/x/sys/unix" "github.com/gorilla/mux" "gopkg.in/check.v1" @@ -1149,29 +1150,45 @@ services: c.Check(tasks[0].Kind(), Equals, "stop") } -func mockShutdownSyscall(f func()) (restore func()) { - old := shutdownSyscall - shutdownSyscall = f +func mockSyncSyscall(f func()) (restore func()) { + old := syncSyscall + syncSyscall = f return func() { - shutdownSyscall = old + syncSyscall = old } } -func mockCheckCapSysBoot(f func() error) (restore func()) { - old := checkCapSysBoot - checkCapSysBoot = f +func mockRebootSyscall(f func(cmd int) error) (restore func()) { + old := rebootSyscall + rebootSyscall = f return func() { - checkCapSysBoot = old + rebootSyscall = old } } +func mockCapGetSyscall(f func(hdr *unix.CapUserHeader, data *unix.CapUserData) error) (restore func()) { + old := capGetSyscall + capGetSyscall = f + return func() { + capGetSyscall = old + } +} + + func (s *daemonSuite) TestSyscallPosRebootDelay(c *C) { wait := make(chan int) - defer mockCheckCapSysBoot(func() error { + defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error { + if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 { + data.Effective = 0xFFFFFFFF + } return nil })() - defer mockShutdownSyscall(func() { - wait <- 1 + defer mockSyncSyscall(func() {})() + defer mockRebootSyscall(func(cmd int) error { + if cmd == unix.LINUX_REBOOT_CMD_RESTART { + wait <- 1 + } + return nil })() period := time.Millisecond * 25 @@ -1189,11 +1206,18 @@ func (s *daemonSuite) TestSyscallPosRebootDelay(c *C) { func (s *daemonSuite) TestSyscallNegRebootDelay(c *C) { wait := make(chan int) - defer mockCheckCapSysBoot(func() error { + defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error { + if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 { + data.Effective = 0xFFFFFFFF + } return nil })() - defer mockShutdownSyscall(func() { - wait <- 1 + defer mockSyncSyscall(func() {})() + defer mockRebootSyscall(func(cmd int) error { + if cmd == unix.LINUX_REBOOT_CMD_RESTART { + wait <- 1 + } + return nil })() // Negative periods will be zeroed, so do not fear the huge negative. @@ -1214,11 +1238,18 @@ func (s *daemonSuite) TestSyscallNegRebootDelay(c *C) { func (s *daemonSuite) TestSetSyscall(c *C) { wait := make(chan int) - defer mockCheckCapSysBoot(func() error { + defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error { + if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 { + data.Effective = 0xFFFFFFFF + } return nil })() - defer mockShutdownSyscall(func() { - wait <- 1 + defer mockSyncSyscall(func() {})() + defer mockRebootSyscall(func(cmd int) error { + if cmd == unix.LINUX_REBOOT_CMD_RESTART { + wait <- 1 + } + return nil })() // We know the default is commandReboot otherwise the unit tests @@ -1240,12 +1271,35 @@ func (s *daemonSuite) TestSetSyscall(c *C) { } func (s *daemonSuite) TestCapSysBootFail(c *C) { - defer mockCheckCapSysBoot(func() error { - return fmt.Errorf("no reboot cap") + defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error { + if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 { + data.Effective = ^(uint32(1 << unix.CAP_SYS_BOOT)) + } + return fmt.Errorf("get cap syscall failed") })() - defer mockShutdownSyscall(func() { - panic("this should not happen") + defer mockSyncSyscall(func() {})() + defer mockRebootSyscall(func(cmd int) error { return nil })() + + // We know the default is commandReboot otherwise the unit tests + // above will fail. We need to check the switch works. + SetSyscallReboot() + defer func() { + rebootHandler = commandReboot + }() + + err := rebootHandler(0) + c.Assert(err, ErrorMatches, "get cap syscall failed") +} + +func (s *daemonSuite) TestCapSysBootNotAvail(c *C) { + defer mockCapGetSyscall(func(hdr *unix.CapUserHeader, data *unix.CapUserData) error { + if hdr.Version == unix.LINUX_CAPABILITY_VERSION_1 { + data.Effective = ^(uint32(1 << unix.CAP_SYS_BOOT)) + } + return nil })() + defer mockSyncSyscall(func() {})() + defer mockRebootSyscall(func(cmd int) error { return nil })() // We know the default is commandReboot otherwise the unit tests // above will fail. We need to check the switch works.