From 38468883574122fffde79591696ad298106177ce Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Wed, 19 Apr 2023 16:35:32 -0700 Subject: [PATCH 01/10] go.mod --- go.mod | 16 ++++++++++++++++ go.sum | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 go.mod create mode 100644 go.sum diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..ed4d424 --- /dev/null +++ b/go.mod @@ -0,0 +1,16 @@ +module github.com/AzureAD/microsoft-authentication-extensions-for-go + +go 1.18 + +require ( + github.com/stretchr/testify v1.8.2 + gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c +) + +require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/kr/pretty v0.2.1 // indirect + github.com/kr/text v0.1.0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..ab0a0eb --- /dev/null +++ b/go.sum @@ -0,0 +1,23 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI= +github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= +github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= +github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= +github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= +github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= +gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From f854cd49c64e46c2a8652a79f1530d9371e38b01 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Wed, 19 Apr 2023 16:38:33 -0700 Subject: [PATCH 02/10] Lock manages file locks --- extensions/internal/lock/lock.go | 75 +++++++++++++++++++ extensions/internal/lock/lock_test.go | 102 ++++++++++++++++++++++++++ 2 files changed, 177 insertions(+) create mode 100644 extensions/internal/lock/lock.go create mode 100644 extensions/internal/lock/lock_test.go diff --git a/extensions/internal/lock/lock.go b/extensions/internal/lock/lock.go new file mode 100644 index 0000000..a69392d --- /dev/null +++ b/extensions/internal/lock/lock.go @@ -0,0 +1,75 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +package lock + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "time" + + "github.com/AzureAD/microsoft-authentication-extensions-for-go/extensions/internal/flock" +) + +// timeout lets tests set the default amount of time allowed to acquire the lock +var timeout = 5 * time.Second + +// flocker helps tests fake flock +type flocker interface { + Fh() *os.File + Path() string + TryLockContext(context.Context, time.Duration) (bool, error) + Unlock() error +} + +// Lock uses a file lock to coordinate access to resources shared with other processes. +// Callers are responsible for preventing races within a process. +type Lock struct { + f flocker + retryDelay time.Duration +} + +// New is the constructor for Lock. "p" is the path to the lock file. +func New(p string, retryDelay time.Duration) (*Lock, error) { + // ensure all dirs in the path exist before flock tries to create the file + err := os.MkdirAll(filepath.Dir(p), os.ModePerm) + if err != nil { + return nil, err + } + return &Lock{f: flock.New(p), retryDelay: retryDelay}, nil +} + +// Lock acquires the file lock on behalf of the process. The behavior of concurrent +// and repeated calls is undefined. For example, Linux may or may not allow goroutines +// scheduled on different threads to hold the lock simultaneously. +func (l *Lock) Lock(ctx context.Context) error { + if _, hasDeadline := ctx.Deadline(); !hasDeadline { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } + locked, err := l.f.TryLockContext(ctx, l.retryDelay) + if err != nil { + return err + } + if locked { + _, _ = l.f.Fh().WriteString(fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0])) + return nil + } + return errors.New("couldn't acquire file lock") +} + +// Unlock releases the lock and deletes the lock file. +func (l *Lock) Unlock() error { + err := l.f.Unlock() + if err == nil { + err = os.Remove(l.f.Path()) + } + if errors.Is(err, os.ErrNotExist) { + return nil + } + return err +} diff --git a/extensions/internal/lock/lock_test.go b/extensions/internal/lock/lock_test.go new file mode 100644 index 0000000..315230d --- /dev/null +++ b/extensions/internal/lock/lock_test.go @@ -0,0 +1,102 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +package lock + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +var ctx = context.Background() + +type fakeFlock struct { + err error + p string +} + +func (f fakeFlock) Fh() *os.File { + fh, _ := os.Open(f.p) + return fh +} + +func (f fakeFlock) Path() string { + return f.p +} + +func (f fakeFlock) TryLockContext(context.Context, time.Duration) (bool, error) { + return f.err == nil, f.err +} + +func (f fakeFlock) Unlock() error { + return f.err +} + +func TestCreatesAndRemovesFile(t *testing.T) { + p := filepath.Join(t.TempDir(), "nonexistent", t.Name()) + lock, err := New(p, 0) + require.NoError(t, err) + require.NoFileExists(t, p) + + err = lock.Lock(ctx) + require.NoError(t, err) + require.FileExists(t, p, "Lock didn't create the file") + + err = lock.Unlock() + require.NoError(t, err) + require.NoFileExists(t, p, "Unlock didn't remove the file") +} + +func TestLockError(t *testing.T) { + p := filepath.Join(t.TempDir(), t.Name()) + lock, err := New(p, 0) + require.NoError(t, err) + expected := errors.New("expected") + lock.f = fakeFlock{err: expected} + require.Equal(t, lock.Lock(ctx), expected) +} + +func TestLockTimeout(t *testing.T) { + defer func(d time.Duration) { timeout = d }(timeout) + timeout = 0 + + p := filepath.Join(t.TempDir(), t.Name()) + a, err := New(p, 0) + require.NoError(t, err) + err = a.Lock(ctx) + require.NoError(t, err) + b, err := New(p, 0) + require.NoError(t, err) + + err = b.Lock(ctx) + require.ErrorIs(t, err, context.DeadlineExceeded) + + require.NoError(t, a.Unlock()) +} + +func TestUnlockErrors(t *testing.T) { + p := filepath.Join(t.TempDir(), t.Name()) + lock, err := New(p, 0) + require.NoError(t, err) + + err = lock.Lock(ctx) + require.NoError(t, err) + if runtime.GOOS != "windows" { + // Remove would fail on Windows because the file lock is mandatory there + require.NoError(t, os.Remove(p)) + } + // Unlock should return nil even when the lock file has been removed + require.NoError(t, lock.Unlock()) + + expected := errors.New("it didn't work") + lock.f = fakeFlock{err: expected} + actual := lock.Unlock() + require.Equal(t, expected, actual) +} From ad316ec0dad608d71f635af5c6555cf217b56d2a Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Wed, 26 Apr 2023 16:44:12 -0700 Subject: [PATCH 03/10] ignore flock errors due to contention --- extensions/internal/lock/lock.go | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/extensions/internal/lock/lock.go b/extensions/internal/lock/lock.go index a69392d..347728f 100644 --- a/extensions/internal/lock/lock.go +++ b/extensions/internal/lock/lock.go @@ -9,6 +9,8 @@ import ( "fmt" "os" "path/filepath" + "runtime" + "syscall" "time" "github.com/AzureAD/microsoft-authentication-extensions-for-go/extensions/internal/flock" @@ -51,15 +53,21 @@ func (l *Lock) Lock(ctx context.Context) error { ctx, cancel = context.WithTimeout(ctx, timeout) defer cancel() } - locked, err := l.f.TryLockContext(ctx, l.retryDelay) - if err != nil { - return err - } - if locked { - _, _ = l.f.Fh().WriteString(fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0])) - return nil + for { + // flock opens the file before locking it and returns errors due to an existing + // lock or one acquired by another process after this process has opened the + // file. We ignore some errors here because in such cases we want to retry until + // the deadline. + locked, err := l.f.TryLockContext(ctx, l.retryDelay) + if err != nil { + if !(errors.Is(err, os.ErrPermission) || isWindowsSharingViolation(err)) { + return err + } + } else if locked { + _, _ = l.f.Fh().WriteString(fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0])) + return nil + } } - return errors.New("couldn't acquire file lock") } // Unlock releases the lock and deletes the lock file. @@ -68,8 +76,13 @@ func (l *Lock) Unlock() error { if err == nil { err = os.Remove(l.f.Path()) } - if errors.Is(err, os.ErrNotExist) { + // ignore errors caused by another process deleting the file or locking between the above Unlock and Remove + if errors.Is(err, os.ErrNotExist) || errors.Is(err, os.ErrPermission) || isWindowsSharingViolation(err) { return nil } return err } + +func isWindowsSharingViolation(err error) bool { + return runtime.GOOS == "windows" && errors.Is(err, syscall.Errno(32)) +} From a2699aafdb32a13d22d42393afd662e64758392b Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Wed, 26 Apr 2023 16:44:41 -0700 Subject: [PATCH 04/10] test fix: remove timeout after first lock --- extensions/internal/lock/lock_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extensions/internal/lock/lock_test.go b/extensions/internal/lock/lock_test.go index 315230d..ed37307 100644 --- a/extensions/internal/lock/lock_test.go +++ b/extensions/internal/lock/lock_test.go @@ -64,14 +64,14 @@ func TestLockError(t *testing.T) { } func TestLockTimeout(t *testing.T) { - defer func(d time.Duration) { timeout = d }(timeout) - timeout = 0 - p := filepath.Join(t.TempDir(), t.Name()) a, err := New(p, 0) require.NoError(t, err) err = a.Lock(ctx) require.NoError(t, err) + + defer func(d time.Duration) { timeout = d }(timeout) + timeout = 0 b, err := New(p, 0) require.NoError(t, err) From 0b9308d1f942bb1a10c242d07b1f873f44179e17 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Thu, 27 Apr 2023 06:00:48 -0700 Subject: [PATCH 05/10] comment on reliability --- extensions/internal/lock/lock.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/extensions/internal/lock/lock.go b/extensions/internal/lock/lock.go index 347728f..eab3081 100644 --- a/extensions/internal/lock/lock.go +++ b/extensions/internal/lock/lock.go @@ -28,7 +28,9 @@ type flocker interface { } // Lock uses a file lock to coordinate access to resources shared with other processes. -// Callers are responsible for preventing races within a process. +// Callers are responsible for preventing races within a process. Lock applies advisory +// locks on Linux and macOS and is therefore unreliable on these platforms when several +// processes concurrently try to acquire the lock. type Lock struct { f flocker retryDelay time.Duration From 8939ff63a1810ecb7ab03f6eb90eae795e11340d Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Mon, 24 Apr 2023 14:58:31 -0700 Subject: [PATCH 06/10] add basic CI --- .github/workflows/go.yml | 36 ++++++++++++++++++++++++++++++++++++ .golangci.yml | 9 +++++++++ 2 files changed, 45 insertions(+) create mode 100644 .github/workflows/go.yml create mode 100644 .golangci.yml diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml new file mode 100644 index 0000000..14bf93f --- /dev/null +++ b/.github/workflows/go.yml @@ -0,0 +1,36 @@ +name: Build and Test + +on: + pull_request: + types: [opened, reopened, synchronize] + push: + branches: [dev, main] + +jobs: + build_test: + strategy: + matrix: + os: [macos-latest, ubuntu-latest, windows-latest] + + runs-on: ${{matrix.os}} + + steps: + - uses: actions/checkout@v3 + + - uses: actions/setup-go@v4 + with: + go-version-file: "go.mod" + + - name: Build + run: go build -x ./extensions/... + + - name: Test + run: go test -race -v ./... + + - name: Lint + # lint even when a previous step failed + if: ${{!cancelled()}} + uses: golangci/golangci-lint-action@v3 + with: + version: v1.52 + args: -v diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..f7f596c --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,9 @@ +linters: + enable: + - gocritic + - gofmt + +linters-settings: + gocritic: + enabled-checks: + - evalOrder From 4e97df134720e9fc268a40d0bb5db6e0b5c0fff5 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Fri, 28 Apr 2023 14:47:27 -0700 Subject: [PATCH 07/10] lint: replace deprecated calls --- extensions/internal/flock/flock_winapi.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/extensions/internal/flock/flock_winapi.go b/extensions/internal/flock/flock_winapi.go index 89b2236..5a40690 100644 --- a/extensions/internal/flock/flock_winapi.go +++ b/extensions/internal/flock/flock_winapi.go @@ -33,9 +33,8 @@ const ( // https://msdn.microsoft.com/en-us/library/windows/desktop/aa365203(v=vs.85).aspx func lockFileEx(handle syscall.Handle, flags uint32, reserved uint32, numberOfBytesToLockLow uint32, numberOfBytesToLockHigh uint32, offset *syscall.Overlapped) (bool, syscall.Errno) { - r1, _, errNo := syscall.Syscall6( + r1, _, errNo := syscall.SyscallN( uintptr(procLockFileEx), - 6, uintptr(handle), uintptr(flags), uintptr(reserved), @@ -55,9 +54,8 @@ func lockFileEx(handle syscall.Handle, flags uint32, reserved uint32, numberOfBy } func unlockFileEx(handle syscall.Handle, reserved uint32, numberOfBytesToLockLow uint32, numberOfBytesToLockHigh uint32, offset *syscall.Overlapped) (bool, syscall.Errno) { - r1, _, errNo := syscall.Syscall6( + r1, _, errNo := syscall.SyscallN( uintptr(procUnlockFileEx), - 5, uintptr(handle), uintptr(reserved), uintptr(numberOfBytesToLockLow), From b682d0f04e00a712ef0b14667be16eb740c17ab4 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Fri, 28 Apr 2023 15:25:18 -0700 Subject: [PATCH 08/10] try auto line endings to work around gofmt false positive --- .gitattributes | 1 + 1 file changed, 1 insertion(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..2125666 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +* text=auto \ No newline at end of file From 23947406aa7ead9a9f609741e8e23db8f6623293 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Fri, 28 Apr 2023 15:40:05 -0700 Subject: [PATCH 09/10] set lf endings for *.go to align with gofmt --- .gitattributes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitattributes b/.gitattributes index 2125666..d207b18 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1 @@ -* text=auto \ No newline at end of file +*.go text eol=lf From 853cb72c51e5897532d44bf4fefcc08ccf777f78 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Wed, 3 May 2023 11:36:50 -0700 Subject: [PATCH 10/10] open the lock file for writing --- extensions/internal/flock/flock.go | 22 ++++--------------- extensions/internal/lock/lock.go | 5 ++++- extensions/internal/lock/lock_test.go | 31 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/extensions/internal/flock/flock.go b/extensions/internal/flock/flock.go index 079f18d..edfdad9 100644 --- a/extensions/internal/flock/flock.go +++ b/extensions/internal/flock/flock.go @@ -19,7 +19,6 @@ package flock import ( "context" "os" - "runtime" "sync" "time" ) @@ -120,24 +119,11 @@ func tryCtx(ctx context.Context, fn func() (bool, error), retryDelay time.Durati } func (f *Flock) setFh() error { - // open a new os.File instance - // create it if it doesn't exist, and open the file read-only. - flags := os.O_CREATE - if runtime.GOOS == "aix" { - // AIX cannot preform write-lock (ie exclusive) on a - // read-only file. - flags |= os.O_RDWR - } else { - flags |= os.O_RDONLY + fh, err := os.OpenFile(f.path, os.O_CREATE|os.O_RDWR, os.FileMode(0600)) + if err == nil { + f.fh = fh } - fh, err := os.OpenFile(f.path, flags, os.FileMode(0600)) - if err != nil { - return err - } - - // set the filehandle on the struct - f.fh = fh - return nil + return err } // ensure the file handle is closed if no lock is held diff --git a/extensions/internal/lock/lock.go b/extensions/internal/lock/lock.go index eab3081..e37210b 100644 --- a/extensions/internal/lock/lock.go +++ b/extensions/internal/lock/lock.go @@ -66,7 +66,10 @@ func (l *Lock) Lock(ctx context.Context) error { return err } } else if locked { - _, _ = l.f.Fh().WriteString(fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0])) + if fh := l.f.Fh(); fh != nil { + s := fmt.Sprintf("{%d} {%s}", os.Getpid(), os.Args[0]) + _, _ = fh.WriteString(s) + } return nil } } diff --git a/extensions/internal/lock/lock_test.go b/extensions/internal/lock/lock_test.go index ed37307..7167eb1 100644 --- a/extensions/internal/lock/lock_test.go +++ b/extensions/internal/lock/lock_test.go @@ -4,8 +4,10 @@ package lock import ( + "bytes" "context" "errors" + "io" "os" "path/filepath" "runtime" @@ -49,11 +51,40 @@ func TestCreatesAndRemovesFile(t *testing.T) { require.NoError(t, err) require.FileExists(t, p, "Lock didn't create the file") + buf := bytes.NewBuffer(nil) + _, err = io.Copy(buf, lock.f.Fh()) + require.NoError(t, err) + require.NotEmpty(t, buf, "Lock didn't write debug info to the locked file") + err = lock.Unlock() require.NoError(t, err) require.NoFileExists(t, p, "Unlock didn't remove the file") } +func TestFileExists(t *testing.T) { + p := filepath.Join(t.TempDir(), t.Name()) + f, err := os.Create(p) + require.NoError(t, err) + data := "stuff" + _, err = f.WriteString(data) + require.NoError(t, err) + require.NoError(t, f.Close()) + + // Lock should succeed when the file exists but isn't locked + lock, err := New(p, 0) + require.NoError(t, err) + err = lock.Lock(ctx) + require.NoError(t, err) + + buf := bytes.NewBuffer(nil) + _, err = io.Copy(buf, lock.f.Fh()) + require.NoError(t, err) + require.NotEqual(t, data, buf, "Lock didn't write debug info to the locked file") + + require.NoError(t, lock.Unlock()) + require.NoFileExists(t, p, "Unlock didn't remove the file") +} + func TestLockError(t *testing.T) { p := filepath.Join(t.TempDir(), t.Name()) lock, err := New(p, 0)