JuliaLang / MbedTLS.jl

Wrapper around mbedtls
Other
41 stars 50 forks source link

fix some subtle bugs with error handling and cleanup #243

Closed vtjnash closed 2 years ago

vtjnash commented 2 years ago

TLS streams can be a bit odd, since they mostly ignore errors on the underlying (unencrypted) stream. Clean up some of that error handling (dealt with in wait_for_encrypted_data, so this actually ends up not passing through mbedtls even though we still could feed it through there), improve flow control reliability (avoids damaging the underlying stream), implement new Base.closewrite API

codecov[bot] commented 2 years ago

Codecov Report

Merging #243 (3d28e8e) into master (0c02e44) will increase coverage by 0.18%. The diff coverage is 20.83%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   71.81%   72.00%   +0.18%     
==========================================
  Files          12       12              
  Lines         699      700       +1     
==========================================
+ Hits          502      504       +2     
+ Misses        197      196       -1     
Impacted Files Coverage Δ
src/ssl.jl 66.90% <20.83%> (-0.24%) :arrow_down:
src/ctr_drbg.jl 85.18% <0.00%> (+1.18%) :arrow_up:
src/entropy.jl 84.61% <0.00%> (+1.28%) :arrow_up:
src/x509_crt.jl 73.91% <0.00%> (+2.48%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0c02e44...3d28e8e. Read the comment docs.

quinnj commented 2 years ago

Looks like closewrite is post-1.6 compatible

vtjnash commented 2 years ago

yes, but I don't bother calling closewrite on the underlying stream, so it is just about making the name right for those versions with a small shim

thchr commented 2 years ago

I bisected the HTTP.jl error from https://github.com/JuliaWeb/HTTP.jl/issues/896 to this commit.

quinnj commented 2 years ago

Yeah, I think we better revert this change unless @vtjnash would have time to look into it sometime soon.

vtjnash commented 2 years ago

You will run into other more frequent deadlocks now if you attempt to revert this, since it does fix some problematic assumptions about stream behaviors. The HTTP says StackOverflowError, so it looks like there is a problem with the handlers there

quinnj commented 2 years ago

The normal HTTP.request path doesn't have any mention of StackOverflow errors. If @vtjnash doesn't have time to investigate the issues that are arising from the changes he made, then I'll revert in the next 24 hours and tag a new release with the commits removed. I haven't seen or been shown specifically (only via vague mentions) code in HTTP.jl that is problematic, but if that can be specifically pointed out, I'm happy to dig in on the HTTP.jl side. I would obviously love to improve the robustness of the MbedTLS code, but we at least weren't having several opened issues pointing directly to the changed code prior to the changes here.

vtjnash commented 2 years ago

Could you write a test for what you think is wrong? This PR passes all tests, and additionally passes the stricter verification for stream-implementation correctness that is now on Julia master

vtjnash commented 2 years ago

Found it and added the missing handling + tested now https://github.com/JuliaLang/MbedTLS.jl/pull/248