-
Notifications
You must be signed in to change notification settings - Fork 128
Fix nil pointer dereference in GetPersistentShell #84
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 nil pointer dereference in GetPersistentShell #84
Conversation
Added nil check in GetPersistentShell before accessing shellInstance.isAlive to prevent panic when newPersistentShell returns nil due to shell startup errors. This resolves the "invalid memory address or nil pointer dereference" error that was occurring in the shell tool.
internal/llm/tools/shell/shell.go
Outdated
@@ -47,7 +47,7 @@ func GetPersistentShell(workingDir string) *PersistentShell { | |||
shellInstance = newPersistentShell(workingDir) | |||
}) | |||
|
|||
if !shellInstance.isAlive { | |||
if shellInstance == nil || !shellInstance.isAlive { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if shellInstance == nil || !shellInstance.isAlive { | |
if shellInstance != nil && !shellInstance.isAlive { |
Is this what we should do instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we want to create a shell instance when it doesn't exist (ie when its nil) or when its not alive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, but we need to change the next line also, we are trying to get .cwd from shellInstance and that shellInstance could be nil in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny thing is I was getting that nil error, but adding that nil check did do the fix. Not sure why it didn't panic on the next line 😆
Will find out the reason and modify it if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kujtimiihoxha I've updated the line to use the provided workingDir if shellInstance is nil, otherwise it will use cwd
330287a
to
a3dc97f
Compare
Thanks @AvicennaJr |
Issue
The application was crashing with a "runtime error: invalid memory address or nil pointer dereference" in the shell tool. This occurred because the GetPersistentShell function was attempting to access properties of a potentially nil shellInstance. Here is the exact panic log:
Root Cause
The newPersistentShell function can return nil in error cases (when failing to create stdin pipe or when cmd.Start() fails), but GetPersistentShell wasn't checking for this possibility before accessing shellInstance.isAlive.
Solution
Added a nil check in GetPersistentShell before accessing shellInstance.isAlive to safely handle cases where shell initialization fails. This prevents the panic and ensures the code attempts to create a new shell instance when needed.