From 05958caf6e858aeff1d2e8e6f8a9a2940f818659 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 17 Jan 2018 23:02:47 +0100 Subject: [PATCH 1/3] sftp: Prompt for password, don't terminate on SIGINT This is a follow-up on fb9729fdb9bf1e15dc6c90383df85a8968f6ddb0, which runs the `ssh` in its own process group and selects that process group as the foreground group. After the sftp connection is established, restic switches back to the previous foreground process group. This allows `ssh` to prompt for the password, but it won't receive the interrupt signal (SIGINT, ^C) later on, because it is not in the foreground process group any more, allowing a clean tear down. --- internal/backend/sftp/foreground_unix.go | 73 +++++++++++++++++++++ internal/backend/sftp/foreground_windows.go | 21 ++++++ internal/backend/sftp/sftp.go | 9 ++- 3 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 internal/backend/sftp/foreground_unix.go create mode 100644 internal/backend/sftp/foreground_windows.go diff --git a/internal/backend/sftp/foreground_unix.go b/internal/backend/sftp/foreground_unix.go new file mode 100644 index 000000000..bdcb52a34 --- /dev/null +++ b/internal/backend/sftp/foreground_unix.go @@ -0,0 +1,73 @@ +// +build !windows + +package sftp + +import ( + "os" + "os/exec" + "os/signal" + "syscall" + "unsafe" + + "github.com/restic/restic/internal/errors" +) + +func tcsetpgrp(fd int, pid int) error { + _, _, errno := syscall.RawSyscall(syscall.SYS_IOCTL, uintptr(fd), + uintptr(syscall.TIOCSPGRP), uintptr(unsafe.Pointer(&pid))) + if errno == 0 { + return nil + } + + return errno +} + +// startForeground runs cmd in the foreground, by temporarily switching to the +// new process group created for cmd. The returned function `bg` switches back +// to the previous process group. +func startForeground(cmd *exec.Cmd) (bg func() error, err error) { + // open the TTY, we need the file descriptor + tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0) + if err != nil { + return nil, errors.Wrap(err, "open TTY") + } + + signal.Ignore(syscall.SIGTTIN) + signal.Ignore(syscall.SIGTTOU) + + // run the command in its own process group + cmd.SysProcAttr = &syscall.SysProcAttr{ + Setpgid: true, + } + + // start the process + err = cmd.Start() + if err != nil { + _ = tty.Close() + return nil, errors.Wrap(err, "cmd.Start") + } + + // move the command's process group into the foreground + prev := syscall.Getpgrp() + err = tcsetpgrp(int(tty.Fd()), cmd.Process.Pid) + if err != nil { + _ = tty.Close() + return nil, err + } + + bg = func() error { + signal.Reset(syscall.SIGTTIN) + signal.Reset(syscall.SIGTTOU) + + // reset the foreground process group + err = tcsetpgrp(int(tty.Fd()), prev) + if err != nil { + _ = tty.Close() + return err + } + + return tty.Close() + } + + return bg, nil +} diff --git a/internal/backend/sftp/foreground_windows.go b/internal/backend/sftp/foreground_windows.go new file mode 100644 index 000000000..e57b4d3a4 --- /dev/null +++ b/internal/backend/sftp/foreground_windows.go @@ -0,0 +1,21 @@ +package sftp + +import ( + "os/exec" + + "github.com/restic/restic/internal/errors" +) + +// startForeground runs cmd in the foreground, by temporarily switching to the +// new process group created for cmd. The returned function `bg` switches back +// to the previous process group. +func startForeground(cmd *exec.Cmd) (bg func() error, err error) { + // just start the process and hope for the best + err = cmd.Start() + if err != nil { + return nil, errors.Wrap(err, "cmd.Start") + } + + bg = func() error { return nil } + return bg, nil +} diff --git a/internal/backend/sftp/sftp.go b/internal/backend/sftp/sftp.go index 911e56293..243258ff3 100644 --- a/internal/backend/sftp/sftp.go +++ b/internal/backend/sftp/sftp.go @@ -69,8 +69,8 @@ func startClient(preExec, postExec func(), program string, args ...string) (*SFT preExec() } - // start the process - if err := cmd.Start(); err != nil { + bg, err := startForeground(cmd) + if err != nil { return nil, errors.Wrap(err, "cmd.Start") } @@ -92,6 +92,11 @@ func startClient(preExec, postExec func(), program string, args ...string) (*SFT return nil, errors.Errorf("unable to start the sftp session, error: %v", err) } + err = bg() + if err != nil { + return nil, errors.Wrap(err, "bg") + } + return &SFTP{c: client, cmd: cmd, result: ch}, nil } From c31a5e7e5cc0524d96f76bdb9c6928bb3f1ebdcd Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 17 Jan 2018 23:14:37 +0100 Subject: [PATCH 2/3] Add argument to Skipf() --- internal/restic/node_unix_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/restic/node_unix_test.go b/internal/restic/node_unix_test.go index a9f0c5df1..4c44d5a34 100644 --- a/internal/restic/node_unix_test.go +++ b/internal/restic/node_unix_test.go @@ -101,7 +101,7 @@ func TestNodeFromFileInfo(t *testing.T) { t.Run("", func(t *testing.T) { fi, found := stat(t, test.filename) if !found && test.canSkip { - t.Skipf("%v not found in filesystem") + t.Skipf("%v not found in filesystem", test.filename) return } From 0bdb131521f84ebce3541f8ccd684c93892b5e66 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 17 Jan 2018 23:14:47 +0100 Subject: [PATCH 3/3] Remove SuspendSignalHandler --- cmd/restic/cleanup.go | 10 ---------- cmd/restic/global.go | 4 ++-- internal/backend/sftp/layout_test.go | 2 +- internal/backend/sftp/sftp.go | 20 ++++++-------------- internal/backend/sftp/sftp_test.go | 4 ++-- 5 files changed, 11 insertions(+), 29 deletions(-) diff --git a/cmd/restic/cleanup.go b/cmd/restic/cleanup.go index daa8ec228..a08127b1d 100644 --- a/cmd/restic/cleanup.go +++ b/cmd/restic/cleanup.go @@ -22,19 +22,9 @@ var stderr = os.Stderr func init() { cleanupHandlers.ch = make(chan os.Signal) go CleanupHandler(cleanupHandlers.ch) - InstallSignalHandler() -} - -// InstallSignalHandler listens for SIGINT, and triggers the cleanup handlers. -func InstallSignalHandler() { signal.Notify(cleanupHandlers.ch, syscall.SIGINT) } -// SuspendSignalHandler removes the signal handler for SIGINT. -func SuspendSignalHandler() { - signal.Reset(syscall.SIGINT) -} - // AddCleanupHandler adds the function f to the list of cleanup handlers so // that it is executed when all the cleanup handlers are run, e.g. when SIGINT // is received. diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 37e3ebea1..81e05d2e3 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -555,7 +555,7 @@ func open(s string, gopts GlobalOptions, opts options.Options) (restic.Backend, // wrap the backend in a LimitBackend so that the throughput is limited be = limiter.LimitBackend(be, limiter.NewStaticLimiter(gopts.LimitUploadKb, gopts.LimitDownloadKb)) case "sftp": - be, err = sftp.Open(cfg.(sftp.Config), SuspendSignalHandler, InstallSignalHandler) + be, err = sftp.Open(cfg.(sftp.Config)) // wrap the backend in a LimitBackend so that the throughput is limited be = limiter.LimitBackend(be, limiter.NewStaticLimiter(gopts.LimitUploadKb, gopts.LimitDownloadKb)) case "s3": @@ -614,7 +614,7 @@ func create(s string, opts options.Options) (restic.Backend, error) { case "local": return local.Create(cfg.(local.Config)) case "sftp": - return sftp.Create(cfg.(sftp.Config), SuspendSignalHandler, InstallSignalHandler) + return sftp.Create(cfg.(sftp.Config)) case "s3": return s3.Create(cfg.(s3.Config), rt) case "gs": diff --git a/internal/backend/sftp/layout_test.go b/internal/backend/sftp/layout_test.go index 0a9e9eae9..db1f1a870 100644 --- a/internal/backend/sftp/layout_test.go +++ b/internal/backend/sftp/layout_test.go @@ -46,7 +46,7 @@ func TestLayout(t *testing.T) { Command: fmt.Sprintf("%q -e", sftpServer), Path: repo, Layout: test.layout, - }, nil, nil) + }) if err != nil { t.Fatal(err) } diff --git a/internal/backend/sftp/sftp.go b/internal/backend/sftp/sftp.go index 243258ff3..7dfa2951e 100644 --- a/internal/backend/sftp/sftp.go +++ b/internal/backend/sftp/sftp.go @@ -36,7 +36,7 @@ var _ restic.Backend = &SFTP{} const defaultLayout = "default" -func startClient(preExec, postExec func(), program string, args ...string) (*SFTP, error) { +func startClient(program string, args ...string) (*SFTP, error) { debug.Log("start client %v %v", program, args) // Connect to a remote host and request the sftp subsystem via the 'ssh' // command. This assumes that passwordless login is correctly configured. @@ -65,19 +65,11 @@ func startClient(preExec, postExec func(), program string, args ...string) (*SFT return nil, errors.Wrap(err, "cmd.StdoutPipe") } - if preExec != nil { - preExec() - } - bg, err := startForeground(cmd) if err != nil { return nil, errors.Wrap(err, "cmd.Start") } - if postExec != nil { - postExec() - } - // wait in a different goroutine ch := make(chan error, 1) go func() { @@ -116,7 +108,7 @@ func (r *SFTP) clientError() error { // Open opens an sftp backend as described by the config by running // "ssh" with the appropriate arguments (or cfg.Command, if set). The function // preExec is run just before, postExec just after starting a program. -func Open(cfg Config, preExec, postExec func()) (*SFTP, error) { +func Open(cfg Config) (*SFTP, error) { debug.Log("open backend with config %#v", cfg) cmd, args, err := buildSSHCommand(cfg) @@ -124,7 +116,7 @@ func Open(cfg Config, preExec, postExec func()) (*SFTP, error) { return nil, err } - sftp, err := startClient(preExec, postExec, cmd, args...) + sftp, err := startClient(cmd, args...) if err != nil { debug.Log("unable to start program: %v", err) return nil, err @@ -209,13 +201,13 @@ func buildSSHCommand(cfg Config) (cmd string, args []string, err error) { // Create creates an sftp backend as described by the config by running "ssh" // with the appropriate arguments (or cfg.Command, if set). The function // preExec is run just before, postExec just after starting a program. -func Create(cfg Config, preExec, postExec func()) (*SFTP, error) { +func Create(cfg Config) (*SFTP, error) { cmd, args, err := buildSSHCommand(cfg) if err != nil { return nil, err } - sftp, err := startClient(preExec, postExec, cmd, args...) + sftp, err := startClient(cmd, args...) if err != nil { debug.Log("unable to start program: %v", err) return nil, err @@ -243,7 +235,7 @@ func Create(cfg Config, preExec, postExec func()) (*SFTP, error) { } // open backend - return Open(cfg, preExec, postExec) + return Open(cfg) } // Location returns this backend's location (the directory name). diff --git a/internal/backend/sftp/sftp_test.go b/internal/backend/sftp/sftp_test.go index 513d6e7d3..f32e04499 100644 --- a/internal/backend/sftp/sftp_test.go +++ b/internal/backend/sftp/sftp_test.go @@ -50,13 +50,13 @@ func newTestSuite(t testing.TB) *test.Suite { // CreateFn is a function that creates a temporary repository for the tests. Create: func(config interface{}) (restic.Backend, error) { cfg := config.(sftp.Config) - return sftp.Create(cfg, nil, nil) + return sftp.Create(cfg) }, // OpenFn is a function that opens a previously created temporary repository. Open: func(config interface{}) (restic.Backend, error) { cfg := config.(sftp.Config) - return sftp.Open(cfg, nil, nil) + return sftp.Open(cfg) }, // CleanupFn removes data created during the tests.