From 49b3c1890334e59d57e1673d9c01d3e3aa860956 Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Wed, 16 Nov 2022 15:21:33 +0100 Subject: [PATCH] GODT-2039: bridge monitors bridge-gui via its PID (port from v2.4) --- internal/app/app.go | 19 +++++-- internal/app/frontend.go | 3 +- internal/bridge/bug_report.go | 10 ++-- .../frontend/bridge-gui/bridge-gui/main.cpp | 5 +- internal/frontend/grpc/service.go | 57 ++++++++++++++++++- internal/frontend/grpc/service_methods.go | 4 ++ pkg/restarter/restarter.go | 26 ++++++++- pkg/restarter/restarter_test.go | 43 ++++++++++++++ tests/ctx_bridge_test.go | 1 + 9 files changed, 153 insertions(+), 15 deletions(-) create mode 100644 pkg/restarter/restarter_test.go diff --git a/internal/app/app.go b/internal/app/app.go index 8916c95c..a894d6b4 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -69,15 +69,16 @@ const ( // Hidden flags. const ( - flagLauncher = "launcher" - flagNoWindow = "no-window" + flagLauncher = "launcher" + flagNoWindow = "no-window" + flagParentPID = "parent-pid" ) const ( appUsage = "Proton Mail IMAP and SMTP Bridge" ) -func New() *cli.App { +func New() *cli.App { //nolint:funlen app := cli.NewApp() app.Name = constants.FullAppName @@ -133,6 +134,11 @@ func New() *cli.App { Usage: "The launcher used to start the app", Hidden: true, }, + &cli.IntFlag{ + Name: flagParentPID, + Usage: "Process ID of the parent", + Hidden: true, + }, } app.Action = run @@ -203,9 +209,14 @@ func run(c *cli.Context) error { //nolint:funlen b.PushError(bridge.ErrVaultCorrupt) } + parentPID := -1 + if pid := c.Int(flagParentPID); pid != 0 { + parentPID = pid + } + // Run the frontend. return runFrontend(c, crashHandler, restarter, - locations, b, eventCh, + locations, b, eventCh, parentPID, ) }, ) diff --git a/internal/app/frontend.go b/internal/app/frontend.go index e5964615..efa35438 100644 --- a/internal/app/frontend.go +++ b/internal/app/frontend.go @@ -38,6 +38,7 @@ func runFrontend( locations *locations.Locations, bridge *bridge.Bridge, eventCh <-chan events.Event, + parentPID int, ) error { switch { case c.Bool(flagCLI): @@ -47,7 +48,7 @@ func runFrontend( select {} case c.Bool(flagGRPC): - service, err := grpc.NewService(crashHandler, restarter, locations, bridge, eventCh, !c.Bool(flagNoWindow)) + service, err := grpc.NewService(crashHandler, restarter, locations, bridge, eventCh, !c.Bool(flagNoWindow), parentPID) if err != nil { return fmt.Errorf("could not create service: %w", err) } diff --git a/internal/bridge/bug_report.go b/internal/bridge/bug_report.go index 50aeb304..de7f9949 100644 --- a/internal/bridge/bug_report.go +++ b/internal/bridge/bug_report.go @@ -36,7 +36,7 @@ const ( MaxCompressedFilesCount = 6 ) -func (bridge *Bridge) ReportBug(ctx context.Context, osType, osVersion, description, username, email, client string, attachLogs bool) error { +func (bridge *Bridge) ReportBug(ctx context.Context, osType, osVersion, description, username, email, client string, attachLogs bool) error { //nolint:funlen var account string if info, err := bridge.QueryUserInfo(username); err == nil { @@ -59,15 +59,15 @@ func (bridge *Bridge) ReportBug(ctx context.Context, osType, osVersion, descript return err } - guiLogs, err := getMatchingLogs(bridge.locator, func(filename string) bool { - return logging.MatchGUILogName(filename) && !logging.MatchStackTraceName(filename) + crashes, err := getMatchingLogs(bridge.locator, func(filename string) bool { + return logging.MatchLogName(filename) && logging.MatchStackTraceName(filename) }) if err != nil { return err } - crashes, err := getMatchingLogs(bridge.locator, func(filename string) bool { - return logging.MatchLogName(filename) && logging.MatchStackTraceName(filename) + guiLogs, err := getMatchingLogs(bridge.locator, func(filename string) bool { + return logging.MatchGUILogName(filename) && !logging.MatchStackTraceName(filename) }) if err != nil { return err diff --git a/internal/frontend/bridge-gui/bridge-gui/main.cpp b/internal/frontend/bridge-gui/bridge-gui/main.cpp index cca6e1cc..4e4dd6f5 100644 --- a/internal/frontend/bridge-gui/bridge-gui/main.cpp +++ b/internal/frontend/bridge-gui/bridge-gui/main.cpp @@ -257,7 +257,10 @@ void launchBridge(QStringList const &args) else app().log().debug(QString("Bridge executable path: %1").arg(QDir::toNativeSeparators(bridgeExePath))); - overseer = std::make_unique(new ProcessMonitor(bridgeExePath, QStringList("--grpc") + args, nullptr), nullptr); + qint64 const pid = qApp->applicationPid(); + QStringList const params = QStringList { "--grpc", "--parent-pid", QString::number(pid) } + args ; + app().log().info(QString("Launching bridge process with command \"%1\" %2").arg(bridgeExePath, params.join(" "))); + overseer = std::make_unique(new ProcessMonitor(bridgeExePath, params , nullptr), nullptr); overseer->startWorker(true); } diff --git a/internal/frontend/grpc/service.go b/internal/frontend/grpc/service.go index e213e2bf..b0b0385b 100644 --- a/internal/frontend/grpc/service.go +++ b/internal/frontend/grpc/service.go @@ -34,6 +34,9 @@ import ( "github.com/ProtonMail/proton-bridge/v2/internal/events" "github.com/ProtonMail/proton-bridge/v2/internal/safe" "github.com/ProtonMail/proton-bridge/v2/internal/updater" + "github.com/bradenaw/juniper/xslices" + "github.com/elastic/go-sysinfo" + sysinfotypes "github.com/elastic/go-sysinfo/types" "github.com/google/uuid" "github.com/sirupsen/logrus" "gitlab.protontech.ch/go/liteapi" @@ -80,8 +83,9 @@ type Service struct { // nolint:structcheck initializing sync.WaitGroup initializationDone sync.Once firstTimeAutostart sync.Once - - showOnStartup bool + parentPID int + parentPIDDoneCh chan struct{} + showOnStartup bool } // NewService returns a new instance of the service. @@ -94,6 +98,7 @@ func NewService( bridge *bridge.Bridge, eventCh <-chan events.Event, showOnStartup bool, + parentPID int, ) (*Service, error) { tlsConfig, certPEM, err := newTLSConfig() if err != nil { @@ -137,7 +142,9 @@ func NewService( initializationDone: sync.Once{}, firstTimeAutostart: sync.Once{}, - showOnStartup: showOnStartup, + parentPID: parentPID, + parentPIDDoneCh: make(chan struct{}), + showOnStartup: showOnStartup, } // Initializing.Done is only called sync.Once. Please keep the increment set to 1 @@ -170,6 +177,12 @@ func (s *Service) initAutostart() { } func (s *Service) Loop() error { + if s.parentPID < 0 { + s.log.Info("Not monitoring parent PID") + } else { + go s.monitorParentPID() + } + defer func() { _ = s.bridge.SetFirstStartGUI(false) }() @@ -486,3 +499,41 @@ func newStreamTokenValidator(wantToken string) grpc.StreamServerInterceptor { return handler(srv, stream) } } + +// monitorParentPID check at regular intervals that the parent process is still alive, and if not shuts down the server +// and the applications. +func (s *Service) monitorParentPID() { + s.log.Infof("Starting to monitor parent PID %v", s.parentPID) + ticker := time.NewTicker(5 * time.Second) + + for { + select { + case <-ticker.C: + if s.parentPID < 0 { + continue + } + + processes, err := sysinfo.Processes() // sysinfo.Process(pid) does not seem to work on Windows. + if err != nil { + s.log.Debug("Could not retrieve process list") + continue + } + + if !xslices.Any(processes, func(p sysinfotypes.Process) bool { return p != nil && p.PID() == s.parentPID }) { + s.log.Info("Parent process does not exist anymore. Initiating shutdown") + // quit will write to the parentPIDDoneCh, so we launch a goroutine. + go func() { + if err := s.quit(); err != nil { + logrus.WithError(err).Error("Error on quit") + } + }() + } else { + s.log.Tracef("Parent process %v is still alive", s.parentPID) + } + + case <-s.parentPIDDoneCh: + s.log.Infof("Stopping process monitoring for PID %v", s.parentPID) + return + } + } +} diff --git a/internal/frontend/grpc/service_methods.go b/internal/frontend/grpc/service_methods.go index a4d7f04a..1f8b8c84 100644 --- a/internal/frontend/grpc/service_methods.go +++ b/internal/frontend/grpc/service_methods.go @@ -104,6 +104,10 @@ func (s *Service) Quit(ctx context.Context, empty *emptypb.Empty) (*emptypb.Empt func (s *Service) quit() error { // Windows is notably slow at Quitting. We do it in a goroutine to speed things up a bit. go func() { + if s.parentPID >= 0 { + s.parentPIDDoneCh <- struct{}{} + } + var err error if s.isStreamingEvents() { if err = s.stopEventStream(); err != nil { diff --git a/pkg/restarter/restarter.go b/pkg/restarter/restarter.go index 59fdcb00..2e9d17e8 100644 --- a/pkg/restarter/restarter.go +++ b/pkg/restarter/restarter.go @@ -72,7 +72,7 @@ func (restarter *Restarter) Restart() { delete(env, BridgeCrashCount) } - cmd := execabs.Command(restarter.exe, xslices.Join(os.Args[1:], restarter.flags)...) //nolint:gosec + cmd := execabs.Command(restarter.exe, xslices.Join(removeFlagWithValue(os.Args[1:], "parent-pid"), restarter.flags)...) //nolint:gosec l := logrus.WithFields(logrus.Fields{ "exe": restarter.exe, "crashCount": env[BridgeCrashCount], @@ -130,3 +130,27 @@ func increment(value string) string { return strconv.Itoa(valueInt + 1) } + +// removeFlagWithValue removes a flag that requires a value from a list of command line parameters. +// The flag can be of the following form `-flag value`, `--flag value`, `-flag=value` or `--flags=value`. +func removeFlagWithValue(argList []string, flag string) []string { + var result []string + + for i := 0; i < len(argList); i++ { + arg := argList[i] + // "detect the parameter form "-flag value" or "--paramName value" + if (arg == "-"+flag) || (arg == "--"+flag) { + i++ + continue + } + + // "detect the form "--flag=value" or "--flag=value" + if strings.HasPrefix(arg, "-"+flag+"=") || (strings.HasPrefix(arg, "--"+flag+"=")) { + continue + } + + result = append(result, arg) + } + + return result +} diff --git a/pkg/restarter/restarter_test.go b/pkg/restarter/restarter_test.go new file mode 100644 index 00000000..dc1e6be3 --- /dev/null +++ b/pkg/restarter/restarter_test.go @@ -0,0 +1,43 @@ +// Copyright (c) 2022 Proton AG +// +// This file is part of Proton Mail Bridge. +// +// Proton Mail Bridge is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Proton Mail Bridge 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 Proton Mail Bridge. If not, see . + +package restarter + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRemoveFlagWithValue(t *testing.T) { + tests := []struct { + argList []string + flag string + expected []string + }{ + {[]string{}, "b", nil}, + {[]string{"-a", "-b=value", "-c"}, "b", []string{"-a", "-c"}}, + {[]string{"-a", "--b=value", "-c"}, "b", []string{"-a", "-c"}}, + {[]string{"-a", "-b", "value", "-c"}, "b", []string{"-a", "-c"}}, + {[]string{"-a", "--b", "value", "-c"}, "b", []string{"-a", "-c"}}, + {[]string{"-a", "-B=value", "-c"}, "b", []string{"-a", "-B=value", "-c"}}, + } + + for _, tt := range tests { + require.Equal(t, removeFlagWithValue(tt.argList, tt.flag), tt.expected) + } +} diff --git a/tests/ctx_bridge_test.go b/tests/ctx_bridge_test.go index 407a0cca..bb8e723d 100644 --- a/tests/ctx_bridge_test.go +++ b/tests/ctx_bridge_test.go @@ -203,6 +203,7 @@ func (t *testCtx) initFrontendService(eventCh <-chan events.Event) error { t.bridge, eventCh, true, + -1, ) if err != nil { return fmt.Errorf("could not create service: %w", err)