search

pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.52k stars 380 forks source link

RequestServer.Serve bugs found looking at PR-361 #363

Closed puellanivis closed 4 years ago

puellanivis commented 4 years ago

So, I end up moving the “tautological statement” into a function under Request (transferError(err error)). The new function better encapsulates behavior of the Request object to calls on the Request object. So, while the check now still exists, it at least is an appropriate guard statement, guarding against potential misbehavior.

The thing I noticed during PR-361, is that the break in the switch { default: … } does not break out of the for { … } loop, but rather only out of the switch. Meaning, the break as it originally appeared was actually a no-op, and execution wasn’t properly ending in that situation.

The solution was to move the for { … } loop into its own independent function, where the loop can then be broken clearly and unambiguously with a return.

Additionally, I noticed that although we’re looping over openRequests, which is supposed to only be accessed under lock, we were not actually holding the lock at that point.

As well, Request.Close really should be closing both the readerAt and the writerAt if they implement, io.Closer, even if the first one returns an err != nil error.

puellanivis commented 4 years ago

No, I don’t think so. There’s a bit of a hope to avoid touching as much as possible, so as little can break as possible. 😆

drakkan commented 4 years ago

I just did a quick test and it seems fine for me, all my test cases passed and also manual checking for transfer errors seems ok, thanks!

puellanivis commented 4 years ago

:grumble-grumble: merge conflicts when a delete and a delete/insert end up head-to-tail next to each other.

nightlyone commented 4 years ago

@eikenb this seems now good to go. Wanna merge it?

puellanivis commented 4 years ago

I’m usually hesitant to commit my own merges if there are no other checks on code. But sure, I think everyone is pretty satisfied that this won’t shouldn’t massively break anything.