GODT-1523: Reduce unnecessary shell executions. Inspired by @kortschak.

- check opened file descriptors
- detection of darwin version
- detection of apple interface color theme
- test for update sync (copy files)
- replace exec with execabs

.gitlab-ci.yml

@@ -180,6 +180,7 @@ build-linux-qa:
     - export PATH=$GOPATH/bin:$PATH
     - export CGO_CPPFLAGS='-Wno-error -Wno-nullability-completeness -Wno-expansion-to-defined -Wno-builtin-requires-header'
   script:
+    - go version
     - make build
     - git diff && git diff-index --quiet HEAD
   cache: {}

cmd/launcher/main.go

@@ -20,7 +20,6 @@ package main
 import (
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"runtime"
 
@@ -36,6 +35,7 @@ import (
 	"github.com/ProtonMail/proton-bridge/v2/internal/versioner"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/execabs"
 )
 
 const (
@@ -98,7 +98,7 @@ func main() { //nolint:funlen
 		logrus.WithError(err).Fatal("Failed to determine path to launcher")
 	}
 
-	cmd := exec.Command(exe, appendLauncherPath(launcher, os.Args[1:])...) //nolint:gosec
+	cmd := execabs.Command(exe, appendLauncherPath(launcher, os.Args[1:])...) //nolint:gosec
 
 	cmd.Stdin = os.Stdin
 	cmd.Stdout = os.Stdout

go.mod

@@ -73,7 +73,7 @@ require (
 	golang.org/x/net v0.0.0-20220425223048-2871e0cb64e4
 	golang.org/x/sys v0.0.0-20220111092808-5a964db01320
 	golang.org/x/text v0.3.7
-	howett.net/plist v1.0.0 // indirect
+	howett.net/plist v1.0.0
 )
 
 replace (

internal/app/base/restart.go

@@ -19,10 +19,10 @@ package base
 
 import (
 	"os"
-	"os/exec"
 	"strconv"
 
 	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/execabs"
 )
 
 // maxAllowedRestarts controls after how many crashes the app will give up restarting.
@@ -43,7 +43,7 @@ func (b *Base) restartApp(crash bool) error {
 		WithField("args", args).
 		Warn("Restarting")
 
-	return exec.Command(b.command, args...).Start() //nolint:gosec
+	return execabs.Command(b.command, args...).Start() //nolint:gosec
 }
 
 // incrementRestartFlag increments the value of the restart flag.

internal/config/tls/cert_store_darwin.go

@@ -17,10 +17,10 @@
 
 package tls
 
-import "os/exec"
+import "golang.org/x/sys/execabs"
 
 func addTrustedCert(certPath string) error {
-	return exec.Command( //nolint:gosec
+	return execabs.Command( //nolint:gosec
 		"/usr/bin/security",
 		"execute-with-privileges",
 		"/usr/bin/security",
@@ -34,7 +34,7 @@ func addTrustedCert(certPath string) error {
 }
 
 func removeTrustedCert(certPath string) error {
-	return exec.Command( //nolint:gosec
+	return execabs.Command( //nolint:gosec
 		"/usr/bin/security",
 		"execute-with-privileges",
 		"/usr/bin/security",

internal/config/useragent/platform.go

@@ -18,7 +18,6 @@
 package useragent
 
 import (
-	"os/exec"
 	"runtime"
 	"strings"
 
@@ -35,20 +34,20 @@ func IsBigSurOrNewer() bool {
 	return isThisDarwinNewerOrEqual(getMinBigSur())
 }
 
-func getMinCatalina() *semver.Version { return semver.MustParse("10.15.0") }
-func getMinBigSur() *semver.Version   { return semver.MustParse("10.16.0") }
+func getMinCatalina() *semver.Version { return semver.MustParse("19.0.0") }
+func getMinBigSur() *semver.Version   { return semver.MustParse("20.0.0") }
 
 func isThisDarwinNewerOrEqual(minVersion *semver.Version) bool {
 	if runtime.GOOS != "darwin" {
 		return false
 	}
 
-	rawVersion, err := exec.Command("sw_vers", "-productVersion").Output()
+	rawVersion, err := getDarwinVersion()
 	if err != nil {
 		return false
 	}
 
-	return isVersionEqualOrNewer(minVersion, strings.TrimSpace(string(rawVersion)))
+	return isVersionEqualOrNewer(minVersion, strings.TrimSpace(rawVersion))
 }
 
 // isVersionEqualOrNewer is separated to be able to run test on other than darwin.

internal/config/useragent/platform_darwin.go

@@ -0,0 +1,29 @@
+// 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 <https://www.gnu.org/licenses/>.
+
+//go:build darwin
+// +build darwin
+
+package useragent
+
+import (
+	"syscall"
+)
+
+func getDarwinVersion() (string, error) {
+	return syscall.Sysctl("kern.osrelease")
+}

internal/config/useragent/platform_default.go

@@ -0,0 +1,27 @@
+// 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 <https://www.gnu.org/licenses/>.
+
+//go:build !darwin
+// +build !darwin
+
+package useragent
+
+import "errors"
+
+func getDarwinVersion() (string, error) {
+	return "", errors.New("implemented only for darwin")
+}

internal/config/useragent/platform_test.go

@@ -25,16 +25,11 @@ import (
 
 func TestIsVersionCatalinaOrNewer(t *testing.T) {
 	testData := map[struct{ version string }]bool{
-		{""}:         false,
-		{"9.0.0"}:    false,
-		{"9.15.0"}:   false,
-		{"10.13.0"}:  false,
-		{"10.14.0"}:  false,
-		{"10.14.99"}: false,
-		{"10.15.0"}:  true,
-		{"10.16.0"}:  true,
-		{"11.0.0"}:   true,
-		{"11.1"}:     true,
+		{""}:       false,
+		{"18.0.0"}: false,
+		{"19.0.0"}: true,
+		{"20.0.0"}: true,
+		{"21.0.0"}: true,
 	}
 
 	for args, exp := range testData {
@@ -45,16 +40,11 @@ func TestIsVersionCatalinaOrNewer(t *testing.T) {
 
 func TestIsVersionBigSurOrNewer(t *testing.T) {
 	testData := map[struct{ version string }]bool{
-		{""}:         false,
-		{"9.0.0"}:    false,
-		{"9.15.0"}:   false,
-		{"10.13.0"}:  false,
-		{"10.14.0"}:  false,
-		{"10.14.99"}: false,
-		{"10.15.0"}:  false,
-		{"10.16.0"}:  true,
-		{"11.0.0"}:   true,
-		{"11.1"}:     true,
+		{""}:       false,
+		{"18.0.0"}: false,
+		{"19.0.0"}: false,
+		{"20.0.0"}: true,
+		{"21.0.0"}: true,
 	}
 
 	for args, exp := range testData {

internal/frontend/clientconfig/config_applemail.go

@@ -23,7 +23,6 @@ package clientconfig
 import (
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -33,6 +32,7 @@ import (
 	"github.com/ProtonMail/proton-bridge/v2/internal/config/useragent"
 	"github.com/ProtonMail/proton-bridge/v2/internal/frontend/types"
 	"github.com/ProtonMail/proton-bridge/v2/pkg/mobileconfig"
+	"golang.org/x/sys/execabs"
 )
 
 const (
@@ -56,10 +56,10 @@ func (c *appleMail) Configure(imapPort, smtpPort int, imapSSL, smtpSSL bool, use
 	}
 
 	if useragent.IsBigSurOrNewer() {
-		return exec.Command("open", bigSurPreferncesPane, confPath).Run() //nolint:gosec G204: open command is safe, mobileconfig is generated by us
+		return execabs.Command("open", bigSurPreferncesPane, confPath).Run() //nolint:gosec G204: open command is safe, mobileconfig is generated by us
 	}
 
-	return exec.Command("open", confPath).Run() //nolint:gosec G204: open command is safe, mobileconfig is generated by us
+	return execabs.Command("open", confPath).Run() //nolint:gosec G204: open command is safe, mobileconfig is generated by us
 }
 
 func prepareMobileConfig(imapPort, smtpPort int, imapSSL, smtpSSL bool, user types.User, address string) *mobileconfig.Config {

internal/frontend/theme/detect_darwin.go

@@ -21,14 +21,34 @@
 package theme
 
 import (
-	"os/exec"
-	"strings"
+	"os"
+	"path/filepath"
+
+	"howett.net/plist"
 )
 
 func detectSystemTheme() Theme {
-	out, err := exec.Command("defaults", "read", "-g", "AppleInterfaceStyle").Output() //nolint:gosec
-	if err == nil && strings.TrimSpace(string(out)) == "Dark" {
+	home, err := os.UserHomeDir()
+	if err != nil {
+		return Light
+	}
+
+	path := filepath.Join(home, "/Library/Preferences/.GlobalPreferences.plist")
+	prefFile, err := os.Open(path)
+	if err != nil {
+		return Light
+	}
+	defer prefFile.Close()
+
+	var data struct {
+		AppleInterfaceStyle string `plist:AppleInterfaceStyle`
+	}
+
+	dec := plist.NewDecoder(prefFile)
+	err = dec.Decode(&data)
+	if err == nil && data.AppleInterfaceStyle == "Dark" {
 		return Dark
 	}
+
 	return Light
 }

internal/smtp/user.go

@@ -313,16 +313,16 @@ func (su *smtpUser) Send(returnPath string, to []string, messageReader io.Reader
 
 	startTime := time.Now()
 	for isSending && time.Since(startTime) < 90*time.Second {
-		log.Debug("Message is still in send queue, waiting for a bit")
+		log.Warn("Message is still in send queue, waiting for a bit")
 		time.Sleep(15 * time.Second)
 		isSending, wasSent = su.backend.sendRecorder.isSendingOrSent(su.client(), sendRecorderMessageHash)
 	}
 	if isSending {
-		log.Debug("Message is still in send queue, returning error to prevent client from adding it to the sent folder prematurely")
+		log.Warn("Message is still in send queue, returning error to prevent client from adding it to the sent folder prematurely")
 		return errors.New("original message is still being sent")
 	}
 	if wasSent {
-		log.Debug("Message was already sent")
+		log.Warn("Message was already sent")
 		return nil
 	}
 

internal/store/ulimit_default.go

@@ -15,33 +15,36 @@
 // You should have received a copy of the GNU General Public License
 // along with Proton Mail Bridge. If not, see <https://www.gnu.org/licenses/>.
 
+//go:build !windows
+// +build !windows
+
 package store
 
 import (
-	"fmt"
-	"os"
-	"os/exec"
 	"runtime"
-	"strconv"
-	"strings"
+	"syscall"
 )
 
-func uLimit() int {
-	if runtime.GOOS != "darwin" && runtime.GOOS != "linux" {
-		return 0
-	}
-	out, err := exec.Command("bash", "-c", "ulimit -n").Output()
+func getCurrentFDLimit() (int, error) {
+	var limits syscall.Rlimit
+	err := syscall.Getrlimit(syscall.RLIMIT_NOFILE, &limits)
 	if err != nil {
-		log.Print(err)
-		return 0
+		return 0, err
 	}
-	outStr := strings.Trim(string(out), " \n")
-	num, err := strconv.Atoi(outStr)
-	if err != nil {
-		log.Print(err)
-		return 0
+	return int(limits.Cur), nil
+}
+
+func countOpenedFDs(limit int) int {
+	openedFDs := 0
+
+	for i := 0; i < limit; i++ {
+		_, _, err := syscall.Syscall(syscall.SYS_FCNTL, uintptr(i), uintptr(syscall.F_GETFL), 0)
+		if err == 0 {
+			openedFDs++
+		}
 	}
-	return num
+
+	return openedFDs
 }
 
 func isFdCloseToULimit() bool {
@@ -49,16 +52,18 @@ func isFdCloseToULimit() bool {
 		return false
 	}
 
-	pid := fmt.Sprint(os.Getpid())
-	out, err := exec.Command("lsof", "-p", pid).Output() //nolint:gosec
+	limit, err := getCurrentFDLimit()
 	if err != nil {
-		log.Warn("isFdCloseToULimit: ", err)
+		log.WithError(err).Error("Cannot get current FD limit")
 		return false
 	}
-	lines := strings.Split(string(out), "\n")
 
-	fd := len(lines) - 1
-	ulimit := uLimit()
-	log.Info("File descriptor check: num goroutines ", runtime.NumGoroutine(), " fd ", fd, " ulimit ", ulimit)
-	return fd >= int(0.95*float64(ulimit))
+	openedFDs := countOpenedFDs(limit)
+
+	log.
+		WithField("noGoroutines", runtime.NumCgoCall()).
+		WithField("noFDs", openedFDs).
+		WithField("limitFD", limit).
+		Info("File descriptor check")
+	return openedFDs >= int(0.95*float64(limit))
 }

internal/store/ulimit_windows.go

@@ -0,0 +1,23 @@
+// 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 <https://www.gnu.org/licenses/>.
+
+//go:build windows
+// +build windows
+
+package store
+
+func isFdCloseToULimit() bool { return false }

internal/updater/sync_test.go

@@ -20,7 +20,6 @@ package updater
 import (
 	"io/ioutil"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"testing"
 
@@ -39,77 +38,103 @@ const (
 func TestSyncFolder(t *testing.T) {
 	for _, srcType := range []string{EmptyType, FileType, SymlinkType, DirType} {
 		for _, dstType := range []string{EmptyType, FileType, SymlinkType, DirType} {
-			require.NoError(t, checkCopyWorks(srcType, dstType))
+			checkCopyWorks(t, srcType, dstType)
 			logrus.Warn("OK: from ", srcType, " to ", dstType)
 		}
 	}
 }
 
-func checkCopyWorks(srcType, dstType string) error {
+func checkCopyWorks(tb testing.TB, srcType, dstType string) {
+	r := require.New(tb)
 	dirName := "from_" + srcType + "_to_" + dstType
 	AppCacheDir := "/tmp"
 	srcDir := filepath.Join(AppCacheDir, "sync_src", dirName)
 	destDir := filepath.Join(AppCacheDir, "sync_dst", dirName)
 
 	// clear before
-	logrus.Info("remove all ", srcDir)
-	err := os.RemoveAll(srcDir)
-	if err != nil {
-		return err
-	}
-
-	logrus.Info("remove all ", destDir)
-	err = os.RemoveAll(destDir)
-	if err != nil {
-		return err
-	}
+	r.NoError(os.RemoveAll(srcDir))
+	r.NoError(os.RemoveAll(destDir))
 
 	// create
-	err = createTestFolder(srcDir, srcType)
-	if err != nil {
-		return err
-	}
-
-	err = createTestFolder(destDir, dstType)
-	if err != nil {
-		return err
-	}
+	r.NoError(createTestFolder(srcDir, srcType))
+	r.NoError(createTestFolder(destDir, dstType))
 
 	// copy
-	logrus.Info("Sync from ", srcDir, " to ", destDir)
-	err = syncFolders(destDir, srcDir)
-	if err != nil {
-		return err
-	}
+	r.NoError(syncFolders(destDir, srcDir))
 
 	// Check
-	logrus.Info("check ", srcDir, " and ", destDir)
-	err = checkThatFilesAreSame(srcDir, destDir)
-	if err != nil {
-		return err
-	}
+	checkThatFilesAreSame(r, srcDir, destDir)
 
 	// clear after
-	logrus.Info("remove all ", srcDir)
-	err = os.RemoveAll(srcDir)
-	if err != nil {
-		return err
-	}
-
-	logrus.Info("remove all ", destDir)
-	err = os.RemoveAll(destDir)
-	if err != nil {
-		return err
-	}
-
-	return err
+	r.NoError(os.RemoveAll(srcDir))
+	r.NoError(os.RemoveAll(destDir))
 }
 
-func checkThatFilesAreSame(src, dst string) error {
-	cmd := exec.Command("diff", "-qr", src, dst) //nolint:gosec
-	cmd.Stderr = logrus.StandardLogger().WriterLevel(logrus.ErrorLevel)
-	cmd.Stdout = logrus.StandardLogger().WriterLevel(logrus.InfoLevel)
-	return cmd.Run()
+func checkThatFilesAreSame(r *require.Assertions, src, dst string) {
+	srcFiles, srcDirs, err := walkDir(src)
+	r.NoError(err)
+
+	dstFiles, dstDirs, err := walkDir(dst)
+	r.NoError(err)
+
+	r.ElementsMatch(srcFiles, dstFiles)
+	r.ElementsMatch(srcDirs, dstDirs)
+
+	for _, relPath := range srcFiles {
+		srcPath := filepath.Join(src, relPath)
+		r.FileExists(srcPath)
+
+		dstPath := filepath.Join(dst, relPath)
+		r.FileExists(dstPath)
+
+		srcInfo, err := os.Lstat(srcPath)
+		r.NoError(err)
+
+		dstInfo, err := os.Lstat(dstPath)
+		r.NoError(err)
+
+		r.Equal(srcInfo.Mode(), dstInfo.Mode())
+
+		if srcInfo.Mode()&os.ModeSymlink == os.ModeSymlink {
+			srcLnk, err := os.Readlink(srcPath)
+			r.NoError(err)
+
+			dstLnk, err := os.Readlink(dstPath)
+			r.NoError(err)
+
+			r.Equal(srcLnk, dstLnk)
+		} else {
+			srcContent, err := ioutil.ReadFile(srcPath)
+			r.NoError(err)
+
+			dstContent, err := ioutil.ReadFile(dstPath)
+			r.NoError(err)
+
+			r.Equal(srcContent, dstContent)
+		}
+	}
+}
+
+func walkDir(dir string) (files, dirs []string, err error) {
+	err = filepath.Walk(dir, func(path string, info os.FileInfo, errWalk error) error {
+		if errWalk != nil {
+			return errWalk
+		}
+
+		relPath, errRel := filepath.Rel(dir, path)
+		if errRel != nil {
+			return errRel
+		}
+
+		if info.IsDir() {
+			dirs = append(dirs, relPath)
+		} else {
+			files = append(files, relPath)
+		}
+
+		return nil
+	})
+	return
 }
 
 func createTestFolder(dirPath, dirType string) error {

pkg/keychain/helper_linux.go

@@ -18,13 +18,13 @@
 package keychain
 
 import (
-	"os/exec"
 	"reflect"
 
 	"github.com/docker/docker-credential-helpers/credentials"
 	"github.com/docker/docker-credential-helpers/pass"
 	"github.com/docker/docker-credential-helpers/secretservice"
 	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/execabs"
 )
 
 const (
@@ -40,11 +40,11 @@ func init() { //nolint:gochecknoinits
 		Helpers[SecretServiceDBus] = newDBusHelper
 	}
 
-	if _, err := exec.LookPath("gnome-keyring"); err == nil && isUsable(newSecretServiceHelper("")) {
+	if _, err := execabs.LookPath("gnome-keyring"); err == nil && isUsable(newSecretServiceHelper("")) {
 		Helpers[SecretService] = newSecretServiceHelper
 	}
 
-	if _, err := exec.LookPath("pass"); err == nil && isUsable(newPassHelper("")) {
+	if _, err := execabs.LookPath("pass"); err == nil && isUsable(newPassHelper("")) {
 		Helpers[Pass] = newPassHelper
 	}
 

test/context/context.go

@@ -175,5 +175,6 @@ func (ctx *TestContext) MessagePreparationFinished(username string) {
 }
 
 func (ctx *TestContext) CredentialsFailsOnWrite(shouldFail bool) {
-	ctx.credStore.(*fakeCredStore).failOnWrite = shouldFail //nolint:forcetypeassert
+	ctx.credStore.(*fakeCredStore).failOnWrite = shouldFail                                              //nolint:forcetypeassert
+	ctx.addCleanup(func() { ctx.credStore.(*fakeCredStore).failOnWrite = false }, "credentials-cleanup") //nolint:forcetypeassert
 }

test/context/credentials.go

@@ -35,7 +35,7 @@ type fakeCredStore struct {
 
 // newFakeCredStore returns a fake credentials store (optionally configured with the given credentials).
 func newFakeCredStore(initCreds ...*credentials.Credentials) (c *fakeCredStore) {
-	c = &fakeCredStore{credentials: map[string]*credentials.Credentials{}}
+	c = &fakeCredStore{credentials: map[string]*credentials.Credentials{}, failOnWrite: false}
 	for _, creds := range initCreds {
 		if creds == nil {
 			continue