-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix deadlock in Git.handleBinary
#3375
Conversation
ab2983d
to
a5e29f6
Compare
I initially chose not to call I'm guessing maybe the command wasn't exiting which is what lead to the rogue processes Dustin might've been trying to address with his PR.
|
It seems like the process still doesn't exit after several hours, yet running the command manually it completes in seconds. From what I've read, it's possible that the buffer is being filled up and FWIW, it doesn't show up as Z in |
I can reliably reproduce this hang by making trufflehog/pkg/handlers/handlers.go Lines 274 to 276 in 40fdf44
This leads me to suspect that the hang is caused by the Read/Write of Edit: furthermore, changing func (s *Git) executeCatFileCmd(cmd *exec.Cmd, filepath string) (io.ReadCloser, error) {
var stderr bytes.Buffer
cmd.Stderr = &stderr
stdout, err := cmd.StdoutPipe()
if err != nil {
return nil, fmt.Errorf("error running git cat-file: %w\n%s", err, stderr.Bytes())
}
if err := cmd.Start(); err != nil {
return nil, fmt.Errorf("error starting git cat-file: %w\n%s", err, stderr.Bytes())
}
// Create or open the file to write the output
fileName := strings.ReplaceAll(filepath, "/", "_")
outFile, err := os.Create("/tmp/thog/" + fileName)
if err != nil {
return nil, fmt.Errorf("error creating file: '%s', %w", filepath, err)
}
defer outFile.Close()
// Copy the command's output to the file
if _, err := io.Copy(outFile, stdout); err != nil {
return nil, fmt.Errorf("error copying output to file: %s, %w", filepath, err)
}
// Wait for the command to finish
if err := cmd.Wait(); err != nil {
return nil, fmt.Errorf("error waiting for command: %w", err)
}
return nil, nil |
Pulling this back into draft because I'm not comfortable that this actually fixes the problem. |
Oh, ignore my approval, looks like we have more to figure out. I introduced this regression with #3339. |
The issue doesn't occur if I revert #3351. I have no idea why. Did that change introduce a regression or was it covering up a latent issue? |
That's what I thought too 😅 . I’ll let Dustin respond. I vaguely remember mentions of processes not being killed, but I could be wrong. |
I still don't understand why adding the call to It seems like the issue is within the handler or bufferedreader code and that Git.handleBinary is just incidental. |
I suspect there's an issue with reading from the stdout pipe and buffering the result. I noticed that reading the entire stdout content at once prevents it from hanging. Ex:
I created some tests to see if I could reproduce it but the tests pass event without consuming the entire reader with the snippet above..
|
I believe Size() consumes the entire reader if it's not seekable. It would hold it in memory if it's small enough otherwise, I think it writes it to disk. |
I can get it to reproduce specifically with this file. This issue isn't going anywhere, so probably something best looked at after a good night's rest.
Updated test code (click to expand)func TestHandleGitCatFileLargeBlob(t *testing.T) {
fileName := "_tokenizer.cpython-38-x86_64-linux-gnu.so"
// Set up a temporary git repository with a large file
gitDir := setupTempGitRepo(t, fileName)
defer os.RemoveAll(gitDir)
cmd := exec.Command("git", "-C", gitDir, "rev-parse", "HEAD")
hashBytes, err := cmd.Output()
assert.NoError(t, err, "Failed to get commit hash")
commitHash := strings.TrimSpace(string(hashBytes))
// Create a pipe to simulate the git cat-file stdout
cmd = exec.Command("git", "-C", gitDir, "cat-file", "blob", fmt.Sprintf("%s:%s", commitHash, fileName))
var stderr bytes.Buffer
cmd.Stderr = &stderr
stdout, err := cmd.StdoutPipe()
assert.NoError(t, err, "Failed to create stdout pipe")
err = cmd.Start()
assert.NoError(t, err, "Failed to start git cat-file command")
ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
defer cancel()
chunkCh := make(chan *sources.Chunk, 1000) // Adjust buffer size as needed
go func() {
defer close(chunkCh)
err := HandleFile(ctx, stdout, &sources.Chunk{}, sources.ChanReporter{Ch: chunkCh}, WithSkipArchives(false))
assert.NoError(t, err, "HandleFile should not return an error")
}()
time.AfterFunc(15*time.Second, func() {
t.Errorf("cat-file command still alive after 15 seconds")
stdout.Close()
})
err = cmd.Wait()
assert.NoError(t, err, "git cat-file command should complete without error")
count := 0
for range chunkCh {
count++
}
expectedChunks := 100 // This needs to be updated
assert.Equal(t, expectedChunks, count, "Number of chunks should match the expected value")
}
func setupTempGitRepo(t *testing.T, archiveName string) string {
tempDir := t.TempDir()
// Initialize the Git repository
cmd := exec.Command("git", "init", tempDir)
var initStderr bytes.Buffer
cmd.Stderr = &initStderr
err := cmd.Run()
if err != nil {
t.Fatalf("Failed to initialize git repository: %v, stderr: %s", err, initStderr.String())
}
archivePath := filepath.Join(tempDir, archiveName)
cmd = exec.Command("cp", "/tmp/_tokenizer.cpython-38-x86_64-linux-gnu.so", archivePath)
var cpStderr bytes.Buffer
cmd.Stderr = &cpStderr
err = cmd.Run()
if err != nil {
t.Fatalf("Failed to copy file: %v, stderr: %s", err, cpStderr.String())
}
// Add the archive to the Git repository
cmd = exec.Command("git", "-C", tempDir, "add", archiveName)
var addStderr bytes.Buffer
cmd.Stderr = &addStderr
err = cmd.Run()
if err != nil {
t.Fatalf("Failed to add archive to git: %v, stderr: %s", err, addStderr.String())
}
cmd = exec.Command("git", "-C", tempDir, "commit", "-m", "Add archive file")
var commitStderr bytes.Buffer
cmd.Stderr = &commitStderr
err = cmd.Run()
if err != nil {
t.Fatalf("Failed to commit archive to git: %v, stderr: %s", err, commitStderr.String())
}
return tempDir
} |
I think this affirms that the issue is with
Edit: if I add some debugging statements to
Change diff (click to expand)diff --git a/pkg/iobuf/bufferedreaderseeker.go b/pkg/iobuf/bufferedreaderseeker.go
index 47ba5119..f70361c9 100644
--- a/pkg/iobuf/bufferedreaderseeker.go
+++ b/pkg/iobuf/bufferedreaderseeker.go
@@ -141,6 +141,8 @@ func (br *BufferedReadSeeker) Read(out []byte) (int, error) {
// If we've exceeded the in-memory threshold and have a temp file.
if br.tempFile != nil && br.index < br.diskBufferSize {
+ fmt.Printf("[BufferedReadSeeker#Read] in-memory threshold exceeded, going to file '%s'\n", br.tempFile.Name())
+
if _, err := br.tempFile.Seek(br.index-int64(br.buf.Len()), io.SeekStart); err != nil {
return totalBytesRead, err
}
@@ -172,9 +174,11 @@ func (br *BufferedReadSeeker) Read(out []byte) (int, error) {
if errors.Is(err, io.EOF) {
br.totalSize = br.bytesRead
+ fmt.Printf("[BufferedReadSeeker#Read] Total size = %d\n", br.bytesRead)
br.sizeKnown = true
}
+ fmt.Printf("[BufferedReadSeeker#Read] Read '%d' bytes, err = %v\n", totalBytesRead, err)
return totalBytesRead, err
} |
I think I've figured it out: the file is skipped due to its extension/mimetype, therefore the reader is never read/drained. trufflehog/pkg/handlers/default.go Lines 77 to 81 in 40fdf44
I had to manually comment out the noisy
|
@ahrav what's the most efficient way to ensure the reader is flushed? I would have thought it was handled by .Close(): trufflehog/pkg/handlers/handlers.go Line 286 in 57802ab
|
I was initially thinking something like:
But i'm not sure that handles all cases given
I'm working on a mechanism to propagate errors from the goroutines back to the caller, but I think using io.Copy should work. |
That looks worthwhile, though it wouldn't fix this specific issue as Either way, I think this can be closed in favour of #3379 + your future change. Edit: I misread the second example. That could work -- however, any future changes or new paths could cause a regression. I think an ideal solution would be to handle it at the top level ( |
I agree. We either need to drain the reader or propagate the error. What do you think? Ideally, I'd like a single place to handle error management, but that might not be practical.
|
After looking through the code, it seems like the best place — for now. Adding test cases for top-level and ignored nested files for each handler should help mitigate anything being missed. |
Description:
This closes #3339.
cmd
andcatCmd
are the same reference. It seems that explicitly callingcmd.Wait()
could cause a deadlock where the deferredstdout.Close()
was never run. Albeit, this could be merely fixing the 'symptom' and not actually curing the issue.Edit: hmm, this seems produce the beloved
signal: broken pipe
error when callingcmd.Wait()
.Checklist:
make test-community
)?make lint
this requires golangci-lint)?