Browse Source

Leave the socket's directory behind when the socket goes

listen remembered each directory it made, in package state under a mutex,
so that the last socket out could remove it. An empty directory costs
nothing, the next start wants it again, and a directory the operator
named is theirs; the tracking and its two tests go.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Matt Richardson 1 day ago
parent
commit
b2ccb7a492
2 changed files with 2 additions and 95 deletions
  1. 2 36
      localapi/platform_unix.go
  2. 0 59
      localapi/platform_unix_test.go

+ 2 - 36
localapi/platform_unix.go

@@ -69,8 +69,8 @@ func listen(path string, acc Access) (net.Listener, error) {
 
 	// One level only. Every directory defaultPathFor offers sits directly under one that
 	// exists, and an operator who names a deeper path creates it; a missing parent is a
-	// mistake to report, not a tree to build. A directory made here is remembered, so
-	// that removeSocket can take it away again once nothing is left in it.
+	// mistake to report, not a tree to build. A directory made here stays when the
+	// socket goes: an empty one costs nothing, and the next start wants it again.
 	//
 	// 0755 gives nothing away: the socket's own mode is the access control. What it
 	// gives is a directory others can look into, which is what a client that is not the
@@ -81,7 +81,6 @@ func listen(path string, acc Access) (net.Listener, error) {
 		if err := os.Chmod(dir, 0755); err != nil {
 			return nil, fmt.Errorf("could not set the mode of %s: %w", dir, err)
 		}
-		madeDirs.add(dir)
 	case !errors.Is(err, os.ErrExist):
 		return nil, fmt.Errorf("could not create %s: %w", dir, err)
 	}
@@ -295,7 +294,6 @@ func removeSocket(path string, id socketID) {
 		return
 	}
 	_ = os.Remove(path)
-	madeDirs.remove(filepath.Dir(path))
 }
 
 // identifySocket returns the identity of the node at path, looked at without following
@@ -312,38 +310,6 @@ func identifySocket(path string) (socketID, bool) {
 	return socketID{dev: uint64(st.Dev), ino: uint64(st.Ino)}, true
 }
 
-// madeDirs is the set of directories listen created and has not yet removed. It is
-// package state because the socket's path is all release and listen share.
-var madeDirs = dirSet{made: map[string]bool{}}
-
-type dirSet struct {
-	mu   sync.Mutex
-	made map[string]bool
-}
-
-func (d *dirSet) add(dir string) {
-	d.mu.Lock()
-	defer d.mu.Unlock()
-	d.made[dir] = true
-}
-
-// remove takes dir away if listen made it and it is empty. One that still holds
-// something, another socket of ours or whatever else arrived, is kept and stays
-// remembered, so the release of the last socket in it is the one that removes it. The
-// same goes for any other failure: a directory that is still there is still ours to
-// remove, and the next release tries again. Only one that is gone is forgotten.
-func (d *dirSet) remove(dir string) {
-	d.mu.Lock()
-	defer d.mu.Unlock()
-	if !d.made[dir] {
-		return
-	}
-	if err := os.Remove(dir); err != nil && !errors.Is(err, os.ErrNotExist) {
-		return
-	}
-	delete(d.made, dir)
-}
-
 func dial(path string, timeout time.Duration) (net.Conn, error) {
 	return net.DialTimeout(network, path, timeout)
 }

+ 0 - 59
localapi/platform_unix_test.go

@@ -76,31 +76,6 @@ func TestListenCreatesOneLevelOnly(t *testing.T) {
 	assert.NoDirExists(t, filepath.Join(base, "missing"))
 }
 
-// TestReleaseRemovesTheDirectoryItMade: a directory listen created goes away with the
-// last socket in it, and one that was already there is left alone, whatever it holds.
-func TestReleaseRemovesTheDirectoryItMade(t *testing.T) {
-	base := privateBase(t)
-
-	made := filepath.Join(base, DirName)
-	first, err := listen(filepath.Join(made, "a.sock"), ownerOnly())
-	require.NoError(t, err)
-	second, err := listen(filepath.Join(made, "b.sock"), ownerOnly())
-	require.NoError(t, err)
-
-	release(first, filepath.Join(made, "a.sock"), socketID{})
-	assert.DirExists(t, made, "b.sock is still in it")
-	release(second, filepath.Join(made, "b.sock"), socketID{})
-	assert.NoDirExists(t, made)
-
-	given := filepath.Join(base, "given")
-	require.NoError(t, os.Mkdir(given, 0755))
-	ln, err := listen(filepath.Join(given, "s.sock"), ownerOnly())
-	require.NoError(t, err)
-	release(ln, filepath.Join(given, "s.sock"), socketID{})
-	assert.DirExists(t, given)
-	assert.NoFileExists(t, filepath.Join(given, "s.sock"))
-}
-
 // TestListenRestoresTheUmask: listen tightens the process umask around bind so the socket
 // node is never created looser than 0600. That is process wide state, so it must come
 // back exactly as it was, on success and on failure alike.
@@ -359,40 +334,6 @@ func TestStopLeavesAStrangersSocketAlone(t *testing.T) {
 	assert.NoError(t, err, "the newer daemon's socket is still in place")
 }
 
-// TestMadeDirStaysRememberedUntilItIsGone: a directory listen created that cannot be
-// removed right now is still ours to remove, so the record survives the failure and the
-// next release tries again. Only a directory that is gone is forgotten.
-func TestMadeDirStaysRememberedUntilItIsGone(t *testing.T) {
-	if os.Geteuid() == 0 {
-		t.Skip("root is not refused by directory permissions")
-	}
-
-	parent := t.TempDir()
-	dir := filepath.Join(parent, "nebula")
-	require.NoError(t, os.Mkdir(dir, 0o755))
-
-	d := dirSet{made: map[string]bool{}}
-	d.add(dir)
-
-	// A parent without write permission refuses the rmdir.
-	require.NoError(t, os.Chmod(parent, 0o500))
-	t.Cleanup(func() { _ = os.Chmod(parent, 0o700) })
-	d.remove(dir)
-	assert.True(t, d.made[dir], "a directory that is still there must stay remembered")
-	assert.DirExists(t, dir)
-
-	require.NoError(t, os.Chmod(parent, 0o700))
-	d.remove(dir)
-	assert.False(t, d.made[dir])
-	assert.NoDirExists(t, dir)
-
-	// One that something else already removed is simply forgotten.
-	gone := filepath.Join(parent, "gone")
-	d.add(gone)
-	d.remove(gone)
-	assert.False(t, d.made[gone])
-}
-
 // TestListenRefusesADirectoryOthersCanWrite: whoever can write the socket's directory
 // can put a socket of their own at the path and answer the clients, who trust the path;
 // so a directory writable by its group or by everyone is refused, and nothing is bound.