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

clear OpenSSL thread error queue each time, might help with certain f… #8103

Closed
wants to merge 1 commit into from

Conversation

rdp
Copy link
Contributor

@rdp rdp commented Aug 20, 2019

…ailure cases.

It wasn't clear if this is absolutely necessary but it does follow the spec:
6d08357

And apparently some people have run into problems if they don't do it this way:
nodejs/node-v0.x-archive#1719

I think if you call it more than once it's supposed to give "more detailed" error messages with each invocation or something along those lines.

@straight-shoota
Copy link
Member

Could you try setup a spec for this?
I think messages should be joined by a line break. And please write to an IO instead of concatenation.

@oprypin
Copy link
Member

oprypin commented Aug 20, 2019

please write to an IO instead of concatenation.

I think the point here is that having multiple errors is extremely unlikely (which is why things work at all as they are). So this would likely make the average slower.

@rdp
Copy link
Contributor Author

rdp commented Aug 20, 2019

Are there specs somewhere else I can see as an example of mocking out LibCrypto? Is that possible? Do you still want an IO? (Yes it's very rare AFAIK).

@straight-shoota
Copy link
Member

I think the point here is that having multiple errors is extremely unlikely (which is why things work at all as they are). So this would likely make the average slower.

In that case, there is still a concatenation because message is initialized to "". Concatenating to an empty string might be optimized, though...

@oprypin
Copy link
Member

oprypin commented Aug 20, 2019

Yes, that should be fixed. Perhaps there's a way to combine 2 different desired properties: "no concatenation by default" and "add newlines". Something like this:

if !error
  error = get_error()
else
  error += "\n" + get_error()
end

@asterite
Copy link
Member

Concatenating to an empty string might be optimized, though...

It is! It returns the non-empty string :-)

@rdp
Copy link
Contributor Author

rdp commented Aug 21, 2019

You'd like an exception whose message contains newlines? Might that mess up logging and loggers or some odd?

@rdp
Copy link
Contributor Author

rdp commented Aug 22, 2019

I had hoped this would fix this bug: #8108 but it turned out it didn't. It might be worth pursuing sometime for "better OpenSSL error messages" but I lack the expertise to finish it at this point, love for anybody else to finish it, cheers!

@rdp rdp closed this Aug 22, 2019
@rdp
Copy link
Contributor Author

rdp commented Nov 7, 2019

After some further research, it appears that it's too dangerous to "trust yourself" to having cleared it perfectly before, and the safest option is to insert a call to ERR_clear_error before any OpenSSL call:
https://stackoverflow.com/questions/18179128/how-to-manage-the-error-queue-in-openssl-ssl-get-error-and-err-get-error
This PR is marginally useful for reporting purposes if anybody wants to finish it off, as well :)

@straight-shoota
Copy link
Member

So do we need a new issue to track this?

@rdp
Copy link
Contributor Author

rdp commented Nov 7, 2019

Yeah.

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