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

wolfssl: clean up wolfcrypt error queue #7594

Closed
wants to merge 1 commit into from
Closed

Conversation

@a5ehren
Copy link
Contributor

@a5ehren a5ehren commented Aug 19, 2021

If wolfSSL is built in certain ways (OPENSSL_EXTRA or Debug), the error
queue gets added on to for each session and never freed. Fix it by
calling ERR_clear_error() like in vtls/openssl when needed. This func
is a no-op in wolfcrypt if the error queue is not enabled.

If wolfSSL is built in certain ways (OPENSSL_EXTRA or Debug), the error
queue gets added on to for each session and never freed. Fix it by
calling ERR_clear_error() like in vtls/openssl when needed. This func
is a no-op in wolfcrypt if the error queue is not enabled.
@bagder
Copy link
Member

@bagder bagder commented Aug 19, 2021

Are you saying that there's some kind of memory leak without this patch, or what exactly is the problem this addresses?

@bagder bagder added the SSL/TLS label Aug 19, 2021
@a5ehren
Copy link
Contributor Author

@a5ehren a5ehren commented Aug 19, 2021

Yeah, but there is only a leak if Wolf is built a certain way (max Openssl compatibility) and there are errors in the SSL_read/write paths.

This patch just makes curl treat the Wolfssl error queue the same way as the OpenSSL one - if it isn’t needed due to Wolf being built normally, the Clear function here is a no-op.

@bagder
Copy link
Member

@bagder bagder commented Aug 19, 2021

But why does it have to clear it all the time? If the OpenSSL code does that it seems like a mistake not worth repeating here. Shouldn't the error queue rather get cleared when the connection closes?

@a5ehren
Copy link
Contributor Author

@a5ehren a5ehren commented Aug 19, 2021

That’s a good question. I’ll poke at the code and see if these others actually matter.

@a5ehren
Copy link
Contributor Author

@a5ehren a5ehren commented Aug 20, 2021

So the call before every SSL_read/write comes from this commit 11 years ago: a0dd9df

Apparently the error stack is shared across all callers of the library, so the extra calls to ERR_clear_error() are to guard against bad behavior in other libraries not cleaning up after themselves and causing cURL to abort the connection instead of returning EAGAIN for non-blocking sockets.

bagder
bagder approved these changes Aug 21, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 21, 2021

Thanks for that research and answer to the question. So let's stick to this model for now.

@bagder bagder closed this in 797bacf Aug 21, 2021
@bagder
Copy link
Member

@bagder bagder commented Aug 21, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants