From b7e2ecfa7a5e48c65599c868809067065af58165 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 8 Jun 2025 17:14:37 +0200 Subject: [PATCH] fix: resolve nil pointer dereference panic in shell tool - Add nil safety checks in PersistentShell.Exec method - Add nil safety checks in PersistentShell.Close method - Improve error handling in GetPersistentShell when shell creation fails - Add better error logging in newPersistentShell - Add comprehensive tests for nil pointer safety - Ensure shell instance returns disabled shell instead of nil when creation fails Fixes panic: runtime error: invalid memory address or nil pointer dereference in agent.Run when shell.Exec is called on a nil or improperly initialized shell instance. --- internal/llm/tools/shell/shell.go | 56 +++++++++-- internal/llm/tools/shell/shell_test.go | 126 +++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 internal/llm/tools/shell/shell_test.go diff --git a/internal/llm/tools/shell/shell.go b/internal/llm/tools/shell/shell.go index 7d3b87e4..eb9ef50c 100644 --- a/internal/llm/tools/shell/shell.go +++ b/internal/llm/tools/shell/shell.go @@ -51,8 +51,21 @@ func GetPersistentShell(workingDir string) *PersistentShell { if shellInstance == nil { shellInstance = newPersistentShell(workingDir) + if shellInstance == nil { + // If we still can't create a shell, return a disabled shell instance + return &PersistentShell{ + isAlive: false, + cwd: workingDir, + } + } } else if !shellInstance.isAlive { - shellInstance = newPersistentShell(shellInstance.cwd) + newShell := newPersistentShell(shellInstance.cwd) + if newShell != nil { + shellInstance = newShell + } else { + // If we can't recreate the shell, mark it as not alive + shellInstance.isAlive = false + } } return shellInstance @@ -61,23 +74,23 @@ func GetPersistentShell(workingDir string) *PersistentShell { func newPersistentShell(cwd string) *PersistentShell { // Get shell configuration from config cfg := config.Get() - + // Default to environment variable if config is not set or nil var shellPath string var shellArgs []string - + if cfg != nil { shellPath = cfg.Shell.Path shellArgs = cfg.Shell.Args } - + if shellPath == "" { shellPath = os.Getenv("SHELL") if shellPath == "" { shellPath = "/bin/bash" } } - + // Default shell args if len(shellArgs) == 0 { shellArgs = []string{"-l"} @@ -88,6 +101,7 @@ func newPersistentShell(cwd string) *PersistentShell { stdinPipe, err := cmd.StdinPipe() if err != nil { + fmt.Fprintf(os.Stderr, "Failed to create stdin pipe for shell: %v\n", err) return nil } @@ -95,6 +109,7 @@ func newPersistentShell(cwd string) *PersistentShell { err = cmd.Start() if err != nil { + fmt.Fprintf(os.Stderr, "Failed to start shell process: %v\n", err) return nil } @@ -174,6 +189,14 @@ echo $EXEC_EXIT_CODE > %s shellQuote(statusFile), ) + if s.stdin == nil { + return commandResult{ + stderr: "Shell stdin is not available", + exitCode: 1, + err: errors.New("shell stdin is not available"), + } + } + _, err := s.stdin.Write([]byte(fullCommand + "\n")) if err != nil { return commandResult{ @@ -269,10 +292,20 @@ func (s *PersistentShell) killChildren() { } func (s *PersistentShell) Exec(ctx context.Context, command string, timeoutMs int) (string, string, int, bool, error) { + // Safety check for nil shell instance + if s == nil { + return "", "Shell instance is nil", 1, false, errors.New("shell instance is nil") + } + if !s.isAlive { return "", "Shell is not alive", 1, false, errors.New("shell is not alive") } + // Safety check for nil commandQueue + if s.commandQueue == nil { + return "", "Shell command queue is not initialized", 1, false, errors.New("shell command queue is not initialized") + } + timeout := time.Duration(timeoutMs) * time.Millisecond resultChan := make(chan commandResult) @@ -288,6 +321,11 @@ func (s *PersistentShell) Exec(ctx context.Context, command string, timeoutMs in } func (s *PersistentShell) Close() { + // Safety check for nil shell instance + if s == nil { + return + } + s.mu.Lock() defer s.mu.Unlock() @@ -295,9 +333,13 @@ func (s *PersistentShell) Close() { return } - s.stdin.Write([]byte("exit\n")) + if s.stdin != nil { + s.stdin.Write([]byte("exit\n")) + } - s.cmd.Process.Kill() + if s.cmd != nil && s.cmd.Process != nil { + s.cmd.Process.Kill() + } s.isAlive = false } diff --git a/internal/llm/tools/shell/shell_test.go b/internal/llm/tools/shell/shell_test.go new file mode 100644 index 00000000..8efe82cc --- /dev/null +++ b/internal/llm/tools/shell/shell_test.go @@ -0,0 +1,126 @@ +package shell + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPersistentShell_NilSafety(t *testing.T) { + t.Run("nil shell instance should not panic", func(t *testing.T) { + var shell *PersistentShell = nil + + // Test Exec with nil shell + stdout, stderr, exitCode, interrupted, err := shell.Exec(context.Background(), "echo test", 1000) + + assert.Equal(t, "", stdout) + assert.Equal(t, "Shell instance is nil", stderr) + assert.Equal(t, 1, exitCode) + assert.False(t, interrupted) + assert.Error(t, err) + assert.Contains(t, err.Error(), "shell instance is nil") + }) + + t.Run("nil shell close should not panic", func(t *testing.T) { + var shell *PersistentShell = nil + + // This should not panic + assert.NotPanics(t, func() { + shell.Close() + }) + }) + + t.Run("shell with nil commandQueue should not panic", func(t *testing.T) { + shell := &PersistentShell{ + isAlive: true, + cwd: "/tmp", + commandQueue: nil, // Explicitly nil + } + + stdout, stderr, exitCode, interrupted, err := shell.Exec(context.Background(), "echo test", 1000) + + assert.Equal(t, "", stdout) + assert.Equal(t, "Shell command queue is not initialized", stderr) + assert.Equal(t, 1, exitCode) + assert.False(t, interrupted) + assert.Error(t, err) + assert.Contains(t, err.Error(), "shell command queue is not initialized") + }) + + t.Run("shell with isAlive false should return error", func(t *testing.T) { + shell := &PersistentShell{ + isAlive: false, + cwd: "/tmp", + } + + stdout, stderr, exitCode, interrupted, err := shell.Exec(context.Background(), "echo test", 1000) + + assert.Equal(t, "", stdout) + assert.Equal(t, "Shell is not alive", stderr) + assert.Equal(t, 1, exitCode) + assert.False(t, interrupted) + assert.Error(t, err) + assert.Contains(t, err.Error(), "shell is not alive") + }) +} + +func TestGetPersistentShell_FailureHandling(t *testing.T) { + t.Run("should return disabled shell when creation fails", func(t *testing.T) { + // This test is tricky because we can't easily force newPersistentShell to fail + // But we can test that GetPersistentShell returns a non-nil shell + shell := GetPersistentShell("/tmp") + + require.NotNil(t, shell) + + // The shell should either be alive or disabled, but not nil + if !shell.isAlive { + // If shell is not alive, it should handle commands gracefully + stdout, stderr, exitCode, interrupted, err := shell.Exec(context.Background(), "echo test", 1000) + + assert.Equal(t, "", stdout) + assert.Equal(t, "Shell is not alive", stderr) + assert.Equal(t, 1, exitCode) + assert.False(t, interrupted) + assert.Error(t, err) + } + }) +} + +func TestShellQuote(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"simple", "'simple'"}, + {"with spaces", "'with spaces'"}, + {"with'quote", "'with'\\''quote'"}, + {"", "''"}, + {"multiple'quotes'here", "'multiple'\\''quotes'\\''here'"}, + } + + for _, test := range tests { + t.Run(test.input, func(t *testing.T) { + result := shellQuote(test.input) + assert.Equal(t, test.expected, result) + }) + } +} + +func TestFileHelpers(t *testing.T) { + t.Run("fileExists should handle non-existent files", func(t *testing.T) { + exists := fileExists("/non/existent/file") + assert.False(t, exists) + }) + + t.Run("fileSize should handle non-existent files", func(t *testing.T) { + size := fileSize("/non/existent/file") + assert.Equal(t, int64(0), size) + }) + + t.Run("readFileOrEmpty should handle non-existent files", func(t *testing.T) { + content := readFileOrEmpty("/non/existent/file") + assert.Equal(t, "", content) + }) +}