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

Call CRYPTO_set_locking_callback #352

Closed
wants to merge 9 commits into from
Closed

Conversation

public
Copy link
Member

@public public commented Dec 26, 2013

So that we actually get some thread safety.

The cross platform locking stuff is based on the PyPy code and is entirely untested on Windows. This does fix the crash in my test case described in #330 on Linux.

@public
Copy link
Member Author

public commented Dec 26, 2013

Do we want a unit test that spawn a few threads and tries to cause bad things to happen? It usually crashes pretty fast on Linux if things aren't working but @reaperhulk said it didn't crash on Windows anyway :-/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 45e673a on public:thread-safe into b645521 on pyca:master.

@jenkins-cryptography
Copy link

Can one of the admins verify this patch?

1 similar comment
@jenkins-cryptography
Copy link

Can one of the admins verify this patch?

@reaperhulk
Copy link
Member

add to whitelist

@reaperhulk
Copy link
Member

I'd definitely like to see a test, but even with #330 it usually takes 20-30 seconds to crash on OS X. We could create a mark for slow tests if we wanted to start splitting this sort of thing out from the primary test suite? Would need to create a jenkins job to run the slow tests exclusively then...

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/68/

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/69/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 057bff4 on public:thread-safe into b645521 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 057bff4 on public:thread-safe into b645521 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 057bff4 on public:thread-safe into b645521 on pyca:master.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/70/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b1f38cf on public:thread-safe into b645521 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling b1f38cf on public:thread-safe into b645521 on pyca:master.

@public
Copy link
Member Author

public commented Dec 26, 2013

I think I can safely get rid of the sem_init version and just use the pthread_mutex on Linux now but I'm not really sure why CPython and PyPy both have implementations for each of them.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5e6949d on public:thread-safe into b645521 on pyca:master.

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/71/

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/72/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling afe1007 on public:thread-safe into b645521 on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 5a178ef on public:thread-safe into b645521 on pyca:master.

@public
Copy link
Member Author

public commented Dec 30, 2013

I've added attempting to import _ssl to get Python to set it's handler. This also conditionally sets our handler if Python didn't for some reason. I'm not sure this buys us anything but making things more complex. _ssl won't reinit it's lock handler ever after the first import so simply immediately overwriting it's handler is quite safe.

I guess we save CRYPTO_num_locks() * sizeof(lock) memory though :)

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/118/

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/119/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 38a853e on public:thread-safe into fbd7ffc on pyca:master.

@jenkins-cryptography
Copy link

Test FAILed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/120/

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 6a3c4de on public:thread-safe into fbd7ffc on pyca:master.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/121/

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.11%) when pulling 6a3c4de on public:thread-safe into fbd7ffc on pyca:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 80108a4 on public:thread-safe into fbd7ffc on pyca:master.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/122/

@alex
Copy link
Member

alex commented Jan 3, 2014

FYI this need to be rebased on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling de6f553 on public:thread-safe into adbea0d on pyca:master.

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/172/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/180/

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 12ffba3 on public:thread-safe into adbea0d on pyca:master.

@public
Copy link
Member Author

public commented Jan 7, 2014

Did you get anywhere with using this in pyOpenSSL @exarkun ?

@hynek
Copy link
Contributor

hynek commented Jan 18, 2014

Given pyca/pyopenssl#7 I believe this should be high-priority.

So that we actually get some thread safety
because their sem_init is implemented as exit(1)
pthread_mutex works on Linux anyway and isn't obviously any better at
coping with fork() issues.
Probably not particularly great at finding issues on all platforms.
Notably Windows doesn't seem to crash without it anyway.
Adds quite a bit of complexity, might be better to set ours
unconditionally but still import _ssl so that it doesn't overwrite ours
at some point later.
Makes it a bit easier to read.
@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/373/

@jenkins-cryptography
Copy link

Test PASSed.
Refer to this link for build results: http://jenkins.cryptography.io/job/cryptography-pr-experimental/374/

@reaperhulk
Copy link
Member

Latest build returns these errors when attempting to compile on windows: http://bpaste.net/show/prfcOee90CjIL85YbuCz/

@public
Copy link
Member Author

public commented Jan 20, 2014

Weird given that it worked before. I think I know what's wrong though, will sort it out once I get a Windows VM to play with :)

@alex
Copy link
Member

alex commented Jan 24, 2014

Closing in favor of #492

@alex alex closed this Jan 24, 2014
joerichter-stash pushed a commit to kiwigrid/cryptography that referenced this pull request Nov 15, 2017
complete the flake8-ification of this file
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

6 participants