Merge pull request #187 from nkryuchkov/fix/pathutil-panic

Fix a panic in pathutil

pkg/util/pathutil/configpath.go

@@ -59,16 +59,18 @@ func (dp ConfigPaths) String() string {
 	if err != nil {
 		log.Fatalf("cannot marshal default paths: %s", err.Error())
 	}
+
 	return string(raw)
 }
 
 // Get obtains a path stored under given configuration location type.
 func (dp ConfigPaths) Get(cpType ConfigLocationType) string {
-	if path, ok := dp[cpType]; ok {
-		return path
+	path, ok := dp[cpType]
+	if !ok {
+		log.Fatalf("invalid config type '%s' provided. Valid types: %v", cpType, AllConfigLocationTypes())
 	}
-	log.Fatalf("invalid config type '%s' provided. Valid types: %v", cpType, AllConfigLocationTypes())
-	return ""
+
+	return path
 }
 
 // VisorDefaults returns the default config paths for skywire-visor.
@@ -77,8 +79,10 @@ func VisorDefaults() ConfigPaths {
 	if wd, err := os.Getwd(); err == nil {
 		paths[WorkingDirLoc] = filepath.Join(wd, "skywire-config.json")
 	}
+
 	paths[HomeLoc] = filepath.Join(HomeDir(), ".skycoin/skywire/skywire-config.json")
 	paths[LocalLoc] = "/usr/local/SkycoinProject/skywire-mainnet/skywire-config.json"
+
 	return paths
 }
 
@@ -88,8 +92,10 @@ func HypervisorDefaults() ConfigPaths {
 	if wd, err := os.Getwd(); err == nil {
 		paths[WorkingDirLoc] = filepath.Join(wd, "hypervisor-config.json")
 	}
+
 	paths[HomeLoc] = filepath.Join(HomeDir(), ".skycoin/hypervisor/hypervisor-config.json")
 	paths[LocalLoc] = "/usr/local/SkycoinProject/hypervisor/hypervisor-config.json"
+
 	return paths
 }
 
@@ -102,29 +108,37 @@ func FindConfigPath(args []string, argsIndex int, env string, defaults ConfigPat
 	if argsIndex >= 0 && len(args) > argsIndex {
 		path := args[argsIndex]
 		log.Infof("using args[%d] as config path: %s", argsIndex, path)
+
 		return path
 	}
+
 	if env != "" {
 		if path, ok := os.LookupEnv(env); ok {
 			log.Infof("using $%s as config path: %s", env, path)
 			return path
 		}
 	}
+
 	log.Debugf("config path is not explicitly specified, trying default paths...")
+
 	for i, cpType := range []ConfigLocationType{WorkingDirLoc, HomeLoc, LocalLoc} {
 		path, ok := defaults[cpType]
 		if !ok {
 			continue
 		}
+
 		if _, err := os.Stat(path); err != nil {
 			log.Debugf("- [%d/%d] '%s' cannot be accessed: %s", i+1, len(defaults), path, err.Error())
 		} else {
 			log.Debugf("- [%d/%d] '%s' is found", i+1, len(defaults), path)
 			log.Printf("using fallback config path: %s", path)
+
 			return path
 		}
 	}
+
 	log.Fatalf("config not found in any of the following paths: %s", defaults.String())
+
 	return ""
 }
 
@@ -136,14 +150,18 @@ func WriteJSONConfig(conf interface{}, output string, replace bool) {
 	if err != nil {
 		log.WithError(err).Fatal("unexpected error, report to dev")
 	}
+
 	if _, err := os.Stat(output); !replace && err == nil {
 		log.Fatalf("file %s already exists, stopping as 'replace,r' flag is not set", output)
 	}
+
 	if err := os.MkdirAll(filepath.Dir(output), 0750); err != nil {
 		log.WithError(err).Fatalln("failed to create output directory")
 	}
+
 	if err := ioutil.WriteFile(output, raw, 0744); err != nil {
 		log.WithError(err).Fatalln("failed to write file")
 	}
+
 	log.Infof("Wrote %d bytes to %s\n%s", len(raw), output, string(raw))
 }

pkg/util/pathutil/homedir.go

@@ -1,13 +1,9 @@
 package pathutil
 
 import (
-	"io/ioutil"
 	"os"
-	"path"
 	"path/filepath"
 	"runtime"
-
-	"github.com/SkycoinProject/dmsg/cipher"
 )
 
 // HomeDir obtains the path to the user's home directory via ENVs.
@@ -18,63 +14,14 @@ func HomeDir() string {
 		if home == "" {
 			home = os.Getenv("USERPROFILE")
 		}
+
 		return home
 	}
+
 	return os.Getenv("HOME")
 }
 
 // VisorDir returns a path to a directory used to store specific visor configuration. Such dir is ~/.skywire/{PK}
-func VisorDir(pk cipher.PubKey) string {
-	return filepath.Join(HomeDir(), ".skycoin", "skywire", pk.String())
-}
-
-// EnsureDir attempts to create given directory, panics if it fails to do so
-func EnsureDir(path string) {
-	if _, err := os.Stat(path); os.IsNotExist(err) {
-		err := os.MkdirAll(path, 0750)
-		if err != nil {
-			panic(err)
-		}
-	}
-}
-
-// AtomicWriteFile creates a temp file in which to write data, then calls syscall.Rename to swap it and write it on
-// filename for an atomic write. On failure temp file is removed and panics.
-func AtomicWriteFile(filename string, data []byte) {
-	dir, name := path.Split(filename)
-	f, err := ioutil.TempFile(dir, name)
-	if err != nil {
-		panic(err)
-	}
-
-	_, err = f.Write(data)
-	if err == nil {
-		err = f.Sync()
-	}
-	if closeErr := f.Close(); err == nil {
-		err = closeErr
-	}
-	if permErr := os.Chmod(f.Name(), 0600); err == nil {
-		err = permErr
-	}
-	if err == nil {
-		err = os.Rename(f.Name(), filename)
-	}
-
-	if err != nil {
-		if err = os.Remove(f.Name()); err != nil {
-			log.WithError(err).Warnf("Failed to remove file %s", f.Name())
-		}
-		panic(err)
-	}
-}
-
-// AtomicAppendToFile calls AtomicWriteFile but appends new data to destiny file
-func AtomicAppendToFile(filename string, data []byte) {
-	oldFile, err := ioutil.ReadFile(filepath.Clean(filename))
-	if err != nil {
-		panic(err)
-	}
-
-	AtomicWriteFile(filename, append(oldFile, data...))
+func VisorDir(pk string) string {
+	return filepath.Join(HomeDir(), ".skycoin", "skywire", pk)
 }

pkg/util/pathutil/util.go

@@ -0,0 +1,61 @@
+package pathutil
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+)
+
+const (
+	tmpSuffix      = ".tmp"
+	ownerRW        = 0600
+	userRWXGroupRX = 0750
+)
+
+// EnsureDir attempts to create given directory, panics if it fails to do so
+func EnsureDir(path string) error {
+	if _, err := os.Stat(path); os.IsNotExist(err) {
+		return os.MkdirAll(path, userRWXGroupRX)
+	}
+
+	return nil
+}
+
+// AtomicWriteFile creates a temp file in which to write data, then calls syscall.Rename to swap it and write it on
+// filename for an atomic write. On failure temp file is removed and panics.
+func AtomicWriteFile(filename string, data []byte) error {
+	tempFilePath := filename + tmpSuffix
+
+	if _, err := os.Stat(filename); err == nil {
+		if err := os.Remove(filename); err != nil {
+			return fmt.Errorf("remove %s: %w", filename, err)
+		}
+	}
+
+	if _, err := os.Stat(tempFilePath); err == nil {
+		if err := os.Remove(filename); err != nil {
+			return fmt.Errorf("remove %s: %w", filename, err)
+		}
+	}
+
+	if err := ioutil.WriteFile(tempFilePath, data, ownerRW); err != nil {
+		return err
+	}
+
+	if err := os.Rename(tempFilePath, filename); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// AtomicAppendToFile calls AtomicWriteFile but appends new data to destiny file
+func AtomicAppendToFile(filename string, data []byte) error {
+	oldFile, err := ioutil.ReadFile(filepath.Clean(filename))
+	if err != nil {
+		return err
+	}
+
+	return AtomicWriteFile(filename, append(oldFile, data...))
+}

pkg/visor/rpc_test.go

@@ -153,20 +153,24 @@ func TestStartStopApp(t *testing.T) {
 		logger:   logging.MustGetLogger("test"),
 		conf:     &visorCfg,
 	}
-	pathutil.EnsureDir(visor.dir())
+
+	require.NoError(t, pathutil.EnsureDir(visor.dir()))
+
 	defer func() {
 		require.NoError(t, os.RemoveAll(visor.dir()))
 	}()
 
-	pm := &appserver.MockProcManager{}
 	appCfg1 := appcommon.Config{
 		Name:         app,
 		SockFilePath: visorCfg.AppServerSockFile,
 		VisorPK:      visorCfg.Visor.StaticPubKey.Hex(),
 		WorkDir:      filepath.Join("", app),
 	}
+
 	appArgs1 := append([]string{filepath.Join(visor.dir(), app)}, apps["foo"].Args...)
 	appPID1 := appcommon.ProcID(10)
+
+	pm := &appserver.MockProcManager{}
 	pm.On("Start", mock.Anything, appCfg1, appArgs1, mock.Anything, mock.Anything).
 		Return(appPID1, testhelpers.NoErr)
 	pm.On("Wait", app).Return(testhelpers.NoErr)

pkg/visor/visor.go

@@ -253,7 +253,10 @@ func (visor *Visor) Start() error {
 		go visor.pty.ServeCLIRequests(ctx)
 	}
 
-	pathutil.EnsureDir(visor.dir())
+	if err := pathutil.EnsureDir(visor.dir()); err != nil {
+		return err
+	}
+
 	visor.closePreviousApps()
 
 	for _, ac := range visor.appsConf {
@@ -297,7 +300,7 @@ func (visor *Visor) Start() error {
 }
 
 func (visor *Visor) dir() string {
-	return pathutil.VisorDir(visor.conf.Visor.StaticPubKey)
+	return pathutil.VisorDir(visor.conf.Visor.StaticPubKey.String())
 }
 
 func (visor *Visor) pidFile() *os.File {
@@ -335,7 +338,9 @@ func (visor *Visor) closePreviousApps() {
 	}
 
 	// empty file
-	pathutil.AtomicWriteFile(pids.Name(), []byte{})
+	if err := pathutil.AtomicWriteFile(pids.Name(), []byte{}); err != nil {
+		visor.logger.WithError(err).Errorf("Failed to empty file %s", pids.Name())
+	}
 }
 
 func (visor *Visor) stopUnhandledApp(name string, pid int) {
@@ -497,7 +502,10 @@ func (visor *Visor) persistPID(name string, pid appcommon.ProcID) {
 		visor.logger.WithError(err).Warn("Failed to close PID file")
 	}
 
-	pathutil.AtomicAppendToFile(pidFName, []byte(fmt.Sprintf("%s %d\n", name, pid)))
+	data := fmt.Sprintf("%s %d\n", name, pid)
+	if err := pathutil.AtomicAppendToFile(pidFName, []byte(data)); err != nil {
+		visor.logger.WithError(err).Warn("Failed to save PID to file")
+	}
 }
 
 // StopApp stops running App.

pkg/visor/visor_test.go

@@ -194,22 +194,25 @@ func TestVisorSpawnApp(t *testing.T) {
 		logger:   logging.MustGetLogger("test"),
 		conf:     &visorCfg,
 	}
-	pathutil.EnsureDir(visor.dir())
+
+	require.NoError(t, pathutil.EnsureDir(visor.dir()))
+
 	defer func() {
 		require.NoError(t, os.RemoveAll(visor.dir()))
 	}()
 
-	pm := &appserver.MockProcManager{}
 	appCfg := appcommon.Config{
 		Name:         app.App,
 		SockFilePath: visorCfg.AppServerSockFile,
 		VisorPK:      visorCfg.Visor.StaticPubKey.Hex(),
 		WorkDir:      filepath.Join("", app.App),
 	}
-	appArgs := append([]string{filepath.Join(visor.dir(), app.App)}, app.Args...)
-	pm.On("Wait", app.App).Return(testhelpers.NoErr)
 
+	appArgs := append([]string{filepath.Join(visor.dir(), app.App)}, app.Args...)
 	appPID := appcommon.ProcID(10)
+
+	pm := &appserver.MockProcManager{}
+	pm.On("Wait", app.App).Return(testhelpers.NoErr)
 	pm.On("Start", mock.Anything, appCfg, appArgs, mock.Anything, mock.Anything).
 		Return(appPID, testhelpers.NoErr)
 	pm.On("Exists", app.App).Return(true)
@@ -243,7 +246,9 @@ func TestVisorSpawnAppValidations(t *testing.T) {
 		logger: logging.MustGetLogger("test"),
 		conf:   c,
 	}
-	pathutil.EnsureDir(visor.dir())
+
+	require.NoError(t, pathutil.EnsureDir(visor.dir()))
+
 	defer func() {
 		require.NoError(t, os.RemoveAll(visor.dir()))
 	}()
@@ -253,18 +258,18 @@ func TestVisorSpawnAppValidations(t *testing.T) {
 			App:  "skychat",
 			Port: 3,
 		}
-		wantErr := "can't bind to reserved port 3"
 
-		pm := &appserver.MockProcManager{}
 		appCfg := appcommon.Config{
 			Name:         app.App,
 			SockFilePath: c.AppServerSockFile,
 			VisorPK:      c.Visor.StaticPubKey.Hex(),
 			WorkDir:      filepath.Join("", app.App),
 		}
-		appArgs := append([]string{filepath.Join(visor.dir(), app.App)}, app.Args...)
 
+		appArgs := append([]string{filepath.Join(visor.dir(), app.App)}, app.Args...)
 		appPID := appcommon.ProcID(10)
+
+		pm := &appserver.MockProcManager{}
 		pm.On("Run", mock.Anything, appCfg, appArgs, mock.Anything, mock.Anything).
 			Return(appPID, testhelpers.NoErr)
 		pm.On("Exists", app.App).Return(false)
@@ -277,8 +282,11 @@ func TestVisorSpawnAppValidations(t *testing.T) {
 		}()
 
 		time.Sleep(100 * time.Millisecond)
+
 		err := <-errCh
 		require.Error(t, err)
+
+		wantErr := "can't bind to reserved port 3"
 		assert.Equal(t, wantErr, err.Error())
 	})