JuliaLang / MbedTLS.jl

Wrapper around mbedtls
Other
41 stars 50 forks source link

Adds a mutex to supposedly multithread-unsafe call #244

Closed nlw0 closed 2 years ago

nlw0 commented 2 years ago

Bringing https://github.com/JuliaCloud/GoogleCloud.jl/pull/48 further up the stream.

MbedTLS seems to have known multi-thread safety issues (https://github.com/Mbed-TLS/mbedtls/issues/3391, https://github.com/Mbed-TLS/mbedtls/issues/3391). Putting some of the ccall calls might allow us to deal with it.

codecov[bot] commented 2 years ago

Codecov Report

Merging #244 (3cf48b2) into master (0c02e44) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   71.81%   71.85%   +0.04%     
==========================================
  Files          12       12              
  Lines         699      700       +1     
==========================================
+ Hits          502      503       +1     
  Misses        197      197              
Impacted Files Coverage Δ
src/pk.jl 80.32% <100.00%> (+0.32%) :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...3cf48b2. Read the comment docs.

nlw0 commented 2 years ago

The question is what are the ccall calls that would need to be in the mutex. Perhaps it's everything that takes a c_rng argument?

quinnj commented 2 years ago

The question is what are the ccall calls that would need to be in the mutex. Perhaps it's everything that takes a c_rng argument?

From the original mbedtls issues, it sounded like the issue was unique to the pk internal datastructure? So maybe we just keep the lock around sign!, encrypt!, and decrypt! for now? (in the pk.jl) file?

quinnj commented 2 years ago

Ok, I fixed the compat and added the additional locks to encrypt/decrypt in https://github.com/JuliaLang/MbedTLS.jl/pull/245. (sorry for the new PR, but I couldn't push to your fork/branch here)