Fix blocking mux reads and RAW download parsing#221
Conversation
| } | ||
|
|
||
| return n, err | ||
| return n, nil |
There was a problem hiding this comment.
This is good as in hindsight this definitely was a mistake
| io.Copy(conn, file) | ||
| } | ||
|
|
||
| func readRawDownloadName(conn net.Conn) (string, error) { |
There was a problem hiding this comment.
This chain of random functions makes it much much harder to reason about what this code is doing
NHAS
left a comment
NHAS
left a comment
There was a problem hiding this comment.
Could you just explain the rationale around why you've changed the function parsing the raw download request
| limit := len(rawDownloadPrefix) + rawDownloadMaxNameLength + 1 | ||
|
|
||
| for { | ||
| fragment, err := reader.ReadSlice('\n') |
| } | ||
|
|
||
| func readRawDownloadRequest(reader *bufio.Reader) (string, error) { | ||
| var request []byte |
There was a problem hiding this comment.
Why are we using both a bufio reader and then also a buffer that we're appending to here?
There was a problem hiding this comment.
Yep, that makes sense. The rationale for changing this was to avoid relying on a single conn.Read() returning the full RAW<name> request. Since this is TCP, the request can be split across reads, and the old code could parse a partial filename.
The \n handling came from the generated raw download command using echo RAW<name>&3, so the normal request is line-delimited. I kept EOF as a valid terminator as well for callers that send RAW<name> without a newline.
That said, I agree the current helper split makes this look more complicated than the protocol needs. I can simplify it so the raw request parsing is easier to follow while keeping the important behavior:
- bounded read
- support split TCP chunks
- accept newline or EOF as the request terminator
- preserve the existing 64-byte filename limit
|
Updated this to simplify the RAW request parsing. The parser now keeps the bounded read and split-TCP-chunk handling, but removes the |
2 participants
Summary
bufferedConnas a prefix replay wrapper and avoid reading from the underlying connection while buffered prefix data is still available..sh,.ps1, raw/binary filenames, split TCP chunks, empty names, and over-length names.Validation
go test -count=1 ./pkg/mux ./internal/server/tcpThis replaces #220 after the source branch was accidentally deleted while cleaning stale branches in my fork.