out-of-bounds panic in buffer copy #45
Conversation
The size check in TsnetGetIps, TsnetErrmsg, and TsnetGetRemoteAddr used an incorrect condition (len(out) < len(s)-1) that failed to account for the NUL terminator. When buflen equaled or was one less than the string length, the code would write past the buffer bounds, causing a panic that terminates the host process.
|
@kitknox I do prefer the length checks written as they are here, but I am not following the commit description. The commit description seems to claim that this patch fixes an out of bounds write, but copy() in Go only copies up to max(len(dest), len(src)) items, and the final write inside the conditional blocks is bound by out[len(out)-1], so I don't see an out of bounds write unless the caller passed a buflen larger than buf. Can you confirm / provide a reproduction of the issue you saw? |
|
@raggi The bug triggers on out[n] = '\x00' outside of the conditional when the condition is false. Reproduction from macOS below. $ ./test_outofbounds goroutine 17 [running, locked to thread]: |
- Fix bounds checking: check if copy() filled buffer (n >= len(out)) instead of incorrect len(out) < len(src)-1 (thank you @kitknox / #45) - Fix nil pointer dereference in 12 functions when getServer fails - Fix syntax error in TsnetDial referencing undefined variable - Simplify getServer to return *server instead of (*server, error) to avoid repetition of fixed bugs Fixes #45
3 participants
The size check in TsnetGetIps, TsnetErrmsg, and TsnetGetRemoteAddr used an incorrect condition (len(out) < len(s)-1) that failed to account for the NUL terminator. When buflen equaled or was one less than the string length, the code would write past the buffer bounds.