Fix race between lock() and AssertLocked()

[Honny1]

Only write l.locked and l.lockType when counter == 0 to prevent race with unsynchronized reads in AssertLocked().

I'm not sure how this should be tested. I wrote a test that tries to hit the unsynchronized R+W, but hitting the exact timing is pretty hard. With enough iterations and goroutines, the chance is better.

Test:

func TestAssertLockedRaceForRLock(t *testing.T) {
	lock, err := getTempLockfile()
	require.NoError(t, err, "creating lock")

	const numGoroutines = 20000
	const numIterations = 500000
	start := make(chan struct{})
	var wg sync.WaitGroup
	wg.Add(numGoroutines)

	for i := 0; i < numGoroutines; i++ {
		go func(id int) {
			defer wg.Done()
			<-start

			lock.RLock()
			defer lock.Unlock()

			for j := 0; j < numIterations; j++ {
				lock.AssertLocked()
				//time.Sleep(time.Nanosecond * 10)
			}
		}(i)
	}

	close(start)
	wg.Wait()
}

Run command: go test -race -v -run TestAssertLockedRaceForRLock

Fixes: containers/podman#27924

[mtrmac]

Thanks! This LGTM but I’d really like more reviewers.

I don’t think adding an always-executed unit test for this is very valuable, certainly not enough to spend minutes of CPU time on every PR.

---

FWIW the test case you wrote triggered a race report within less than a second for me, on macOS.

A perhaps more direct test case:

func TestAssertLockedRaceForRLock(t *testing.T) {
	lock, err := getTempLockfile()
	require.NoError(t, err, "creating lock")

	lock.RLock()
	defer lock.Unlock()

	const numGoroutines = 100
	const numIterations = 100000
	start := make(chan struct{})
	var wg sync.WaitGroup

	for range numGoroutines / 2 {
		wg.Go(func() {
			<-start

			for range numIterations {
				lock.RLock()
				lock.Unlock()
			}
		})
	}
	for range numGoroutines / 2 {
		wg.Go(func() {
			<-start

			for range numIterations {
				lock.AssertLocked()
			}
		})
	}

	close(start)
	wg.Wait()
}

where the ~important part is that we keep the lock read-locked for the whole duration of the concurrent part, so we can have some goroutines constantly hammer the writer and some constantly hammer the reader.

[mtrmac]

(Nit: I’d aesthetically prefer removing this empty line.)

[TomSweeneyRedHat]

A failing test that I restarted, otherwise
LGTM