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

OpenSSL should clear error queue before each call #8456

Open
rdp opened this issue Nov 8, 2019 · 4 comments
Open

OpenSSL should clear error queue before each call #8456

rdp opened this issue Nov 8, 2019 · 4 comments

Comments

@rdp
Copy link
Contributor

rdp commented Nov 8, 2019

Appears the "best practice" for OpenSSL is to call ERR_clear_error before doing any call, basically "just in case" an earlier SSL_xx call didn't handle its error return value(s) right.

https://stackoverflow.com/questions/18179128/how-to-manage-the-error-queue-in-openssl-ssl-get-error-and-err-get-error

related discussion:

#8103 (comment)

@rdp
Copy link
Contributor Author

rdp commented Nov 9, 2019

I would say that this helps it "be better for multi threaded" as well, but, on closer examination, for SSL_Accept it calls underlying #read multiple times underneath. That means if there are multiple fibers doing SSL calls within the same thread, the thread local error queue could get "cross threaded" with failure messages. I would propose some kind of thread local mutex so only one fiber can be within an openSSL "anyting" at a time but that kind of ruins the point...which makes me wonder if OpenSSL is a good paradigm for the M:N fibers to threads paradigm that Crystal is moving toward has...but anyway this will be slightly better :)

@rdp
Copy link
Contributor Author

rdp commented Nov 23, 2019

I think what should happen is that when there is a fiber context switch, it should "save away" any current openssl failures (and restore from the incoming fiber). Also OpenSSL_Shutdown should clear the queue. FWIW...the latter being the easier of the two, I'll hopefully do a PR for it sometime :)

@RX14
Copy link
Contributor

RX14 commented Nov 28, 2019

@rdp at the moment, fibers can only change threads at specific points. As long as none of these occur between setting an error and reading it, it should be fine.

@rdp
Copy link
Contributor Author

rdp commented Nov 29, 2019

Yeah typically that's enough. Unfortunately sometimes OpenSSL makes several calls to underlying socket #read and #write which all add up to "one failure" and it can store multiple failures messages on the stack (each #read or #write I assume is a context switch point) so if multiple were to fail at the same time they could get cross twisted...

ref: https://stackoverflow.com/q/18179128/32453 comment

Now that you mention it, if we were to make these "thread local" messages basically "fiber local" then...we could call ERR_clear_error before every call to guarantee a clear stack. The original request :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants