Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a mutex to supposedly multithread-unsafe call #244

Closed
wants to merge 1 commit into from

Conversation

nlw0
Copy link
Contributor

@nlw0 nlw0 commented Jul 17, 2022

Bringing JuliaCloud/GoogleCloud.jl#48 further up the stream.

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

@codecov
Copy link

codecov bot commented Jul 17, 2022

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%) ⬆️

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
Copy link
Contributor Author

nlw0 commented Jul 17, 2022

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?

@err_check ccall((:mbedtls_pk_sign, libmbedcrypto), Cint,
(Ptr{Cvoid}, Cint, Ptr{UInt8}, Csize_t, Ptr{UInt8}, Ref{Csize_t}, Ptr{Cvoid}, Any),
ctx.data, hash_alg, hash, sizeof(hash), output, outlen_ref, c_rng[], rng)
@lock MBEDTLSLOCK begin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that @lock wasn't defined until Julia 1.7; so we'll either need to add a compat like:

if !isdefined(Base, Symbol("@lock"))
macro lock(l, expr)
    quote
        temp = $(esc(l))
        lock(temp)
        try
            $(esc(expr))
        finally
            unlock(temp)
        end
    end
end
end

or unroll the macro yourself.

@quinnj
Copy link
Member

quinnj commented Jul 18, 2022

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
Copy link
Member

quinnj commented Jul 18, 2022

Ok, I fixed the compat and added the additional locks to encrypt/decrypt in #245. (sorry for the new PR, but I couldn't push to your fork/branch here)

@quinnj quinnj closed this Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants