Merge pull request #6 from SimonRichardson/acquire-root

#6

The following commit attempts to detect if you're executing with
sudo and to assign the lock file with the correct permissions, to
prevent cryptic error messages.

The code detects sudo using environment variables and correctly
parses the UID and GID from those values, before chown'ing the file.

If at any point there is an error whilst changing the owner, it
will continue as root and on the next run, it will give information
about the file in question. This should then aid diagnosing the
problem without changing the underlying existing flow.

CC: @howbazaar
Background information: https://bugs.launchpad.net/juju/+bug/1758369

Makefile

@@ -1,11 +1,20 @@
-default: check
+PROJECT := github.com/juju/mutex
 
-check:
-	go test
+.PHONY: check-licence check-go check
 
-docs:
-	godoc2md github.com/juju/mutex > README.md
-	sed -i 's|\[godoc-link-here\]|[![GoDoc](https://godoc.org/github.com/juju/mutex?status.svg)](https://godoc.org/github.com/juju/mutex)|' README.md 
+check: check-licence check-go
+	go test $(PROJECT)/...
 
+check-licence:
+	@(fgrep -rl "Licensed under the LGPLv3" .;\
+		fgrep -rl "MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT" .;\
+		find . -name "*.go") | sed -e 's,\./,,' | sort | uniq -u | \
+		xargs -I {} echo FAIL: licence missed: {}
 
-.PHONY: default check docs
+check-go:
+	$(eval GOFMT := $(strip $(shell gofmt -l .| sed -e "s/^/ /g")))
+	@(if [ x$(GOFMT) != x"" ]; then \
+		echo go fmt is sad: $(GOFMT); \
+		exit 1; \
+	fi )
+	@(go tool vet -all -composites=false -copylocks=false .)

dependencies.tsv

@@ -0,0 +1,12 @@
+github.com/golang/mock	git	69521b3833175dfcfb1cc1fdb0c9be92e66faa81	2018-04-03T23:54:22Z
+github.com/juju/errors	git	1b5e39b83d1835fa480e0c2ddefb040ee82d58b3	2015-09-16T12:56:42Z
+github.com/juju/loggo	git	8232ab8918d91c72af1a9fb94d3edbe31d88b790	2017-06-05T01:46:07Z
+github.com/juju/retry	git	62c62032529169c7ec02fa48f93349604c345e1f	2015-10-29T02:48:21Z
+github.com/juju/testing	git	72703b1e95eb8ce4737fd8a3d8496c6b0be280a6	2018-05-17T13:41:05Z
+github.com/juju/utils	git	2000ea4ff0431598aec2b7e1d11d5d49b5384d63	2018-04-24T09:41:59Z
+github.com/juju/version	git	1f41e27e54f21acccf9b2dddae063a782a8a7ceb	2016-10-31T05:19:06Z
+golang.org/x/crypto	git	650f4a345ab4e5b245a3034b110ebc7299e68186	2018-02-14T00:00:28Z
+golang.org/x/net	git	61147c48b25b599e5b561d2e9c4f3e1ef489ca41	2018-04-06T21:48:16Z
+gopkg.in/check.v1	git	4f90aeace3a26ad7021961c297b22c42160c7b25	2016-01-05T16:49:36Z
+gopkg.in/mgo.v2	git	f2b6f6c918c452ad107eec89615f074e3bd80e33	2016-08-18T01:52:18Z
+gopkg.in/yaml.v2	git	1be3d31502d6eabc0dd7ce5b0daab022e14a5538	2017-07-12T05:45:46Z

export_test.go

@@ -0,0 +1,9 @@
+// Copyright 2016-2017 Canonical Ltd.
+// Licensed under the LGPLv3, see LICENCE file for details.
+
+package mutex
+
+var (
+	// Export the Envars so we can test it via mocks
+	Envars = &envars
+)

mutex_flock.go

@@ -8,6 +8,7 @@ package mutex
 import (
 	"os"
 	"path/filepath"
+	"strconv"
 	"sync"
 	"syscall"
 	"time"
@@ -31,6 +32,8 @@ func acquire(spec Spec, timeout <-chan time.Time) (Releaser, error) {
 	}
 }
 
+var envars Environment = osEnvironment{}
+
 var (
 	mu     sync.Mutex
 	active = make(map[string][]*waiter)
@@ -67,8 +70,43 @@ func acquireFlock(name string, done <-chan struct{}) <-chan acquireResult {
 	}
 
 	flockName := filepath.Join(os.TempDir(), "juju-"+name)
-	flock := func() (Releaser, error) {
+	chownFromRoot := func() error {
+		if cmd, ok := envars.LookupEnv("SUDO_COMMAND"); ok && cmd != "" {
+			var uid, gid int
+			uid, err := strconv.Atoi(envars.Getenv("SUDO_UID"))
+			if err != nil {
+				return errors.Annotate(err, "parsing SUDO_UID")
+			}
+			gid, err = strconv.Atoi(envars.Getenv("SUDO_GID"))
+			if err != nil {
+				return errors.Annotate(err, "parsing SUDO_GID")
+			}
+			return syscall.Chown(flockName, uid, gid)
+		}
+		return nil
+	}
+	open := func() (int, error) {
 		fd, err := syscall.Open(flockName, syscall.O_CREAT|syscall.O_RDONLY|syscall.O_CLOEXEC, 0600)
+		if err != nil {
+			if os.IsPermission(err) {
+				err = errors.Annotatef(err, "unable to open %s", flockName)
+			}
+			return fd, err
+		}
+		// Attempting to open a lock file as root whilst using sudo can cause
+		// the lock file to have the wrong permissions for a non-sudo user.
+		// Subsequent calls to acquire the lock file can then lead to cryptic
+		// error messages. Let's attempt to help people out, either by
+		// correcting the permissions, or explaining why we can't help them.
+		// info: lp 1758369
+		if chownErr := chownFromRoot(); chownErr != nil {
+			// The file has the wrong permissions, but we should let the acquire
+			// continue.
+		}
+		return fd, err
+	}
+	flock := func() (Releaser, error) {
+		fd, err := open()
 		if err != nil {
 			return nil, errors.Trace(err)
 		}
@@ -136,3 +174,28 @@ func (m *mutex) Release() {
 	}
 	m.fd = 0
 }
+
+// Environment defines a simple interface with interacting with environmental
+// variables.
+//go:generate mockgen -package mutex_test -destination mutex_mock_test.go github.com/juju/mutex Environment
+type Environment interface {
+
+	// LookupEnv retrieves the value of the environment variable named
+	// by the key.
+	LookupEnv(string) (string, bool)
+
+	// Getenv retrieves the value of the environment variable named by the key.
+	Getenv(string) string
+}
+
+// osEnvironment provides a default way to access environmental values to the
+// acquire method.
+type osEnvironment struct{}
+
+func (osEnvironment) LookupEnv(key string) (string, bool) {
+	return os.LookupEnv(key)
+}
+
+func (osEnvironment) Getenv(key string) string {
+	return os.Getenv(key)
+}

mutex_test.go

@@ -8,12 +8,18 @@ import (
 	"net"
 	"os"
 	"os/exec"
+	"os/user"
+	"path/filepath"
 	"runtime"
 	"sync"
 	"sync/atomic"
+	"syscall"
 	"testing"
 	"time"
 
+	"github.com/golang/mock/gomock"
+
+	jt "github.com/juju/testing"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -284,6 +290,61 @@ func (s *mutexSuite) TestMutexNotInherited(c *gc.C) {
 	r.Release()
 }
 
+// TestFilePermissions tests that the file permissions are correct.
+func (s *mutexSuite) TestFilePermissions(c *gc.C) {
+	spec := s.spec()
+	r, err := mutex.Acquire(spec)
+	c.Assert(err, jc.ErrorIsNil)
+	defer r.Release()
+
+	filePath := filepath.Join(os.TempDir(), "juju-"+spec.Name)
+	fileInfo, err := os.Stat(filePath)
+	c.Assert(err, jc.ErrorIsNil)
+
+	stat, ok := fileInfo.Sys().(*syscall.Stat_t)
+	c.Assert(ok, jc.IsTrue)
+
+	current, err := user.Current()
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(fmt.Sprintf("%d", stat.Uid), gc.Equals, current.Uid)
+	c.Assert(fmt.Sprintf("%d", stat.Gid), gc.Equals, current.Gid)
+}
+
+// TestFilePermissionsWithSudo tests that the file permissions are correct.
+func (s *mutexSuite) TestFilePermissionsWithSudoEnvars(c *gc.C) {
+	ctrl := gomock.NewController(c)
+	defer ctrl.Finish()
+
+	envion := NewMockEnvironment(ctrl)
+
+	restore := jt.PatchValue(mutex.Envars, envion)
+	defer restore()
+
+	current, err := user.Current()
+	c.Assert(err, jc.ErrorIsNil)
+
+	exp := envion.EXPECT()
+	exp.LookupEnv("SUDO_COMMAND").Return("test", true)
+	exp.Getenv("SUDO_UID").Return(current.Uid)
+	exp.Getenv("SUDO_GID").Return(current.Gid)
+
+	spec := s.spec()
+	r, err := mutex.Acquire(spec)
+	c.Assert(err, jc.ErrorIsNil)
+	defer r.Release()
+
+	filePath := filepath.Join(os.TempDir(), "juju-"+spec.Name)
+	fileInfo, err := os.Stat(filePath)
+	c.Assert(err, jc.ErrorIsNil)
+
+	stat, ok := fileInfo.Sys().(*syscall.Stat_t)
+	c.Assert(ok, jc.IsTrue)
+
+	c.Assert(fmt.Sprintf("%d", stat.Uid), gc.Equals, current.Uid)
+	c.Assert(fmt.Sprintf("%d", stat.Gid), gc.Equals, current.Gid)
+}
+
 // LockFromAnotherProc will launch a process and block until that process has
 // created the lock file.  If we time out waiting for the other process to take
 // the lock, this function will fail the current test.
@@ -445,7 +506,7 @@ func TestSleepFromOtherProcess(t *testing.T) {
 	addr := os.Getenv("MUTEX_TEST_HELPER_ADDR")
 	_, err := net.Dial("tcp", addr)
 	if err != nil {
-		fmt.Fprintln(os.Stderr, "error notifying primary process: %v", err)
+		fmt.Fprintf(os.Stderr, "error notifying primary process: %v \n", err)
 		os.Exit(1)
 	}
 	time.Sleep(time.Minute)