Fix "MbedTLS.SSLContext does not support byte I/O"

[albestro]

Close #169

In particular see In https://github.com/JuliaWeb/WebSockets.jl/issues/169#issuecomment-758198121 for additional details

This PR started by reverting to the "old" working code, but it may evolve differently if there is any potentially better solution. Moreover, it would be valuable to find a way for testing this case.

[albestro]

I don't have experience in this field, but I will give it a try.

The comment in the code snippet points out that the connection may be closed abruptly by the other endpoint, resulting in the code trying to access a not existing element, but, at least to me, it does not seem to be linked with the decision on reading 1/2 bytes.

AFAICT, the main reason behind the reverted change reading the first two bytes separately tries to reflect better the RFC6455.

Unfortunately, read(::MbedTLS.SSLContext, ::UInt8) is not provided (indeed, the error message is printed by the fallback instance defined in Base.io). As stated here https://github.com/JuliaLang/MbedTLS.jl/issues/197#issuecomment-472662107, performance may suffer with single byte reads, so it is suggested to use a buffer instead.

I don't know if there is any desire/reason to not just leave the code the old way.

About the test, I don't have any clue on where to start from, looking forward for any idea. In the meanwhile, it may be worth merging this revert and opening an issue about the specific test case it would be nice to add.

[hustf]

Thanks so far! I didn't expect this already. Perhaps we can release a fix version today?

From RFC6455, any frame following protocol needs to be 2 bytes plus. I suppose the worst outcome we could have from receiving one byte would be that the task would lock up indefinitely. Sending one byte and continue to open new connections could be a way to stop normal use of that server without using large resources on the attack side.

Improving tests is better done separately, but it would be nice if you could also revert the corresponding changes to error behaviour. From appveyor messages, that's in test\error_test.jl:117, 118, 145...

[albestro]

I don't know if you may like it or not, it's just a proposal.

I investigated the options we have about reading from the MbedTLS.SSLContext, and I gave a look at readbytes!. AFAICT from the doc this function is non-blocking, moreover it is based on ssl_unsafe_read which in turn it is presented as non blocking...so I'm pretty confident it is what we are looking for.

It returns the number of bytes that have been read, so my proposal is to check that 2 bytes have been read otherwise throw the exception that @SimonDanisch used in its version that we are slightly changing.

About the error in the tests you pointed out, they were curiously due to a BoundsError exception thrown...and with this proposal the problem has been removed.

Another problem: locally on my mac, I had some problems run the tests. In particular, the tests client_listen and client_serverWS were hanging semi-randomly (either with master, 1.5.6 and the current branch).

Let's see how it goes with the CI.

Looking forward for your feedback.

[hustf]

Hey, sorry for the delay - didn't notice the update. I checked this out locally on Windows / Julia 1.5 and got unrelated errors due to a default color change which I thought was covered by PR #165. I'm giving the mac some updates now before replicating your tests there and a closer look on the code.

[hustf]

So, I ran the tests on Mac with version 1.5.3, and no failures. That's with Catalina installed. There's unrelated trouble in the windows, though. Ref. issue #171 and PR #165.

[hustf]

Close #169

[hustf]

Still not sure if this is the best way to release a new version, but Project.toml does no longer contain the version number: @JuliaRegistrator register

[JuliaRegistrator]

Comments on pull requests will not trigger Registrator, as it is disabled. Please try commenting on a commit or issue.

[hustf]

Ok...