Fix potential nil and panics #3067
Conversation
ACEx86
changed the title
|
From just a quick look, it seems most of these changes don't actually fix panics, just move them to later in the functions. How many of these were suggested by AI? |
dnscrypt-proxy/xtransport.go
Outdated
| ips = append(ips, answer.(*dns.A).A.Addr.AsSlice()) | ||
| case dns.TypeAAAA: | ||
| ips = append(ips, answer.(*dns.AAAA).AAAA.Addr.AsSlice()) | ||
| if msg != nil { |
There was a problem hiding this comment.
This could probably be simplified to if msg == nil -> continue instead. Either way, agree this needs a nil check of some kind.
|
None of the suggested panic fixes is by AI i just came accross some of the program crashes that had to do with the deferer. |
dnscrypt-proxy/resolve.go
Outdated
| client := &dns.Client{Transport: transport} | ||
| msg := dns.NewMsg(qName, qType) | ||
| if msg == nil { | ||
| return nil, errors.New("Query message error.") |
There was a problem hiding this comment.
As per the dns.NewMsg docs, nil is returned when the type is unknown so we should be able to say that in the error message. Otherwise agree a nil check is needed here.
Interesting, I'm not sure how some of those values could become I assume it's not as simple as checking for |
If you close the thread or the handle what will happen? |
I'm not sure what you mean by closing the thread or the handle in this context. If you mean some action external to the Go program, I don't see how that would set a value to Are you actually seeing panics at all these places with the deferred close or is it just defensive programming? |
| } | ||
| defer pc.Close() | ||
| defer func(pc net.Conn) { | ||
| if pc != nil { |
There was a problem hiding this comment.
The only possible way I can see this being nil here is if net.DialTimeout returned nil, nil. If we don't trust net.DialTimeout then it's simpler to check at the same time as if err != nil above.
| } | ||
| defer pc.Close() | ||
| defer func(pc net.Conn) { | ||
| if pc != nil { |
There was a problem hiding this comment.
This has more potential for an unknown proxy's Dial function to incorrectly return nil, nil. If that's the case, again the simpler guard is to check for nil at the same time as if err != nil above.
| func (proxy *Proxy) localDoHListener(acceptPc *net.TCPListener) { | ||
| defer acceptPc.Close() | ||
| defer func(acceptPc *net.TCPListener) { | ||
| if acceptPc != nil { |
There was a problem hiding this comment.
In this case, if for some reason acceptPc is nil then there's no point doing anything in localDoHListener. The simpler solution here is an early return.
| } | ||
| defer fSrc.Close() | ||
| defer func(fSrc *safefile.File) { | ||
| if fSrc != nil { |
There was a problem hiding this comment.
If safefile.Create returns nil, nil for whatever reason, it is again simpler to check before the defer call and return some new error instead of continuing to attempt to write anything. I don't see any other code that would change the value of either fSrc or fSig after each safefile.Create call.
| for i, file := range files { | ||
| defer file.Close() | ||
| defer func(file *os.File) { | ||
| if file != nil { |
There was a problem hiding this comment.
Once again, nothing seems to set the value of file to nil within the loop. If it's nil at the start of the loop for whatever reason, we can just continue before the defer instead of trying to register a listener at all.
There was a problem hiding this comment.
The 'defined' execution isn't the only specified thing that can happen.
I don't know if this will result to a panic on linux but if you rm the file after use and before return what will happen trying to close without nil check? And i mean rm it from the system not the program.
There was a problem hiding this comment.
file here is unrelated to any Linux file handle, it will not magically become nil without something within the Go program setting it so.
2 participants
No description provided.