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 mutex to SHA256withRSA for multi-threading #48

Closed
wants to merge 7 commits into from

Conversation

nlw0
Copy link
Collaborator

@nlw0 nlw0 commented Jul 8, 2022

MbedTLS has thread-safety issues (Mbed-TLS/mbedtls#3391, Mbed-TLS/mbedtls#3391), affecting GoogleCloud.jl. This lock seems to solve the problem.

MbedTLS has thread-safety issues (Mbed-TLS/mbedtls#3391, Mbed-TLS/mbedtls#3391), affecting GoogleCloud.jl. This lock seems to solve the problem.
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2022

Codecov Report

Merging #48 (85fedef) into master (53474f7) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master     #48      +/-   ##
=========================================
- Coverage    0.68%   0.68%   -0.01%     
=========================================
  Files          10      10              
  Lines         436     440       +4     
=========================================
  Hits            3       3              
- Misses        433     437       +4     
Impacted Files Coverage Δ
src/session.jl 0.00% <0.00%> (ø)

Nicolau Leal Werneck added 2 commits July 8, 2022 17:56
src/session.jl Outdated Show resolved Hide resolved
src/session.jl Outdated Show resolved Hide resolved
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

The general idea seems good; I wonder though if we should just put this global lock in MbedTLS.jl itself, since others may run into this problem as well? We could make a similar PR to that package and then bump the minimum required version here.

nlw0 and others added 2 commits July 17, 2022 10:05
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
Co-authored-by: Jacob Quinn <quinn.jacobd@gmail.com>
@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 17, 2022

@quinnj sounds good to me, although here it's pretty easy to isolate where to put the lock, and back in MbedTLS.jl we'll probably like to find other spots?... Anyways, I created a PR over there too. JuliaLang/MbedTLS.jl#244

@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 18, 2022

@quinnj I suppose we'll still need to pull the latest MbedTLS here now? How can we do that?

@mattBrzezinski
Copy link
Member

@quinnj I suppose we'll still need to pull the latest MbedTLS here now? How can we do that?

You can set the max version in the Project.toml, JuliaDocs for how to do set the compat.

@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 21, 2022

The change to @lock ... actually breaks this patch in a major way, because output does not get defined. I learned it in the bad way...

@nlw0
Copy link
Collaborator Author

nlw0 commented Jul 22, 2022

@mattBrzezinski MbedTLS@1.1.1 has the fix, so I suppose nothing is really necessary in this package. I have tested running my project without locks in GoogleCloud.jl, only loading MbedTLS@1.1.1, and that seems to solve it. If anything we might set the minimum version but I guess having a workaround with a compatible library version is enough. I'll just go ahead and close this. Thanks everyone!

@mattBrzezinski
Copy link
Member

@mattBrzezinski MbedTLS@1.1.1 has the fix, so I suppose nothing is really necessary in this package. I have tested running my project without locks in GoogleCloud.jl, only loading MbedTLS@1.1.1, and that seems to solve it. If anything we might set the minimum version but I guess having a workaround with a compatible library version is enough. I'll just go ahead and close this. Thanks everyone!

You could create a PR to set the minimum version in the compat section here to 1.1.1, and not have to worry about it. Or upstream in whatever packages you're making set it there as well. You definitely have a few options, but if this is fundamentally broken for this package we should probably fix it here.

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.

4 participants