File descriptor leak on early return in WriteToken (command/agentproxyshared/sink/file/file_sink.go)

[cliffordfingers]

Description:

In command/agentproxyshared/sink/file/file_sink.go (function WriteToken), a file descriptor can be leaked on an early-return error path.

The file is opened:

tmpFile, err := os.OpenFile(filepath.Join(targetDir, ...), os.O_WRONLY|os.O_CREATE, f.mode)

There is an early return before the file is closed:

return fmt.Errorf("error changing ownership of %s: %w", tmpFile.Name(), err)

If osutil.Chown fails, the function returns without calling tmpFile.Close(), leaking the file descriptor on that error path.

Other error paths in the same function (e.g., write error path) correctly call tmpFile.Close(), so this appears to be an inconsistent early-return path that missed the close.

To Reproduce

Code pattern (simplified):

tmpFile, err := os.OpenFile(...)
if err != nil {
return err
}

if err := osutil.Chown(...); err != nil {
return fmt.Errorf("error changing ownership: %w", err)
}

// later:
tmpFile.Close()

If osutil.Chown returns an error, the function exits before tmpFile.Close() is reached.

Expected behavior

The file descriptor should be closed on all return paths.

Specifically, tmpFile.Close() should be guaranteed before any early return after a successful os.OpenFile, either by:

registering defer tmpFile.Close() immediately after the open, or

explicitly closing the file before each early return.

Environment

Vault Server Version (retrieve with vault status):
N/A (static code analysis of current main branch)

Vault CLI Version (retrieve with vault version):
N/A

Server Operating System/Architecture:
N/A

Vault server configuration file(s):
N/A

Additional context

This is a static code issue in the current repository source.
It does not depend on runtime configuration and is reproducible by inspection of the WriteToken implementation.

[dosubot]

I can confirm this is a valid bug. Looking at the WriteToken function, the file descriptor leak exists exactly as you described.

The Chown error path at lines 121-123 returns without closing tmpFile:

if err := osutil.Chown(tmpFile, f.owner, f.group); err != nil {
    return fmt.Errorf("error changing ownership of %s: %w", tmpFile.Name(), err)
}

Meanwhile, the WriteString error path at lines 130-136 correctly handles cleanup:

_, err = tmpFile.WriteString(valToWrite)
if err != nil {
    tmpFile.Close()
    os.Remove(tmpFile.Name())
    return fmt.Errorf("error writing to %s: %w", tmpFile.Name(), err)
}

The fix should add tmpFile.Close() and os.Remove(tmpFile.Name()) before the Chown error return, or alternatively refactor to use defer tmpFile.Close() immediately after opening the file.

We will escalate this to our engineering teams.

Written by @dosu, edited by heatherezell

To reply, just mention @dosu.

---

^{How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other}