fix handshake #52
fix handshake #52
Conversation
|
Is there any problem with this pull request? |
|
I improved the code by reusing the ReadAddr function |
|
You should try to reduce memory allocation with |
|
It allocates only the needed memory. So actually in most cases It allocates less memory than MaxReqLen, but more frequently. So which one is more important? Less memory or less allocation frequency? |
|
Memory allocation is done in page size typically 4KB. Even if you allocate just 1 byte, you still waste 4KB. |
|
OK. I get it! I will rewrite the code. Thanks for reminding me that. |
|
Done! Please review the code again. |
socks/socks.go
Outdated
|
|
||
| _, err = rw.Write([]byte{5, 0}) // SOCKS v5, no auth required | ||
| if err != nil { | ||
| if buf[0] != 5 { // VER: 5 |
There was a problem hiding this comment.
No need to be very strict about SOCKS version. SOCKS4 is about the same. In general the program should be less picky about its input.
socks/socks.go
Outdated
| } | ||
| } | ||
| if !noAuthExist { | ||
| return nil, fmt.Errorf("SOCKS no method noAuth: %v", buf) |
There was a problem hiding this comment.
Again, no need to be strict if the auth method is not specified.
socks/socks.go
Outdated
| if buf[1] != CmdConnect { | ||
| return nil, ErrCommandNotSupported | ||
| } | ||
| buf = buf[3:] | ||
| var err error | ||
| switch buf[0] { |
There was a problem hiding this comment.
On the second thought, you were right in the previous commit. It's actually better to call ReadAddr here instead of duplicating the same error-prone code path.
|
OK. Please review again. |
socks/socks.go
Outdated
| @@ -162,30 +159,33 @@ func ParseAddr(s string) Addr { | |||
| } | |||
|
|
|||
| // Handshake fast-tracks SOCKS initialization to get target address to connect. | |||
| // Read RFC 1928 for request and reply structure and sizes. | |||
There was a problem hiding this comment.
This line of comment is implementation detail. It should NOT be at the function level because it'll be exposed by Godoc.
socks/socks.go
Outdated
|
|
||
| _, err := rw.Read(buf) // SOCKS version and auth methods | ||
| if err != nil { | ||
| buf := make([]byte, 0xff) |
There was a problem hiding this comment.
The magic number 0xff should be defined as a meaningful constant. Reuse MaxReqLen should not hurt (remember the min 4KB page size).
There was a problem hiding this comment.
If I reuse the ReadAddr I will allocate memory twice. Once here for 255 which is the maximum
of byte, Once inside the ReadAddr function for MaxAddrLen. Is that OK?
Or I can pass the buffer into ReadAddr, which will change the ReadAddr function.
There was a problem hiding this comment.
Good point. I thought he double allocation a tradeoff in order to avoid code duplication. There's a way to avoid double allocation and code duplication though:
- Write a helper internal function e.g.
readAddrthat takes a buffer argument. - Rewrite the exposed function
ReadAddrto callreadAddrwith a new buffer. - Call
readAddrinHandshakewith the existing buffer.
In general we do not change the signature of exposed functions in Go to be backward-compatible.
socks/socks.go
Outdated
|
|
||
| _, err = rw.Write([]byte{5, 0}) // SOCKS v5, no auth required | ||
| if err != nil { | ||
| len := buf[1] |
There was a problem hiding this comment.
len is a builtin function. Use another name e.g. nmethod.
|
I use array instead of slice to control the buffer size and reduce memory allocation. Please review again. |
|
No need to use array. In fact it's pretty rare to use arrays in Go. A slice would be just fine. Also, please note there's a bug fix that just got merged which affects this code too. |
|
OK. Please review again. |
socks/socks.go
Outdated
| b := make([]byte, MaxAddrLen) | ||
| func readAddr(r io.Reader, b []byte) (Addr, error) { | ||
| if len(b) < MaxAddrLen { | ||
| return nil, fmt.Errorf("readAddr needs at least %d bytes of buffer", MaxAddrLen) |
There was a problem hiding this comment.
Return io.ErrShortBuffer here instead of generating a custom error string.
|
OK. Please review again. |
|
Thanks a lot! |
Use io.ReadAll to do SOCKS handshake.