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

Use ERR_get_error to get last SSL error in openssl.d #796

Merged
merged 1 commit into from Sep 27, 2014

Conversation

Projects
None yet
2 participants
@cifvts
Contributor

cifvts commented Aug 29, 2014

Getting last error should return an error code which is easier to understand
when translated into a error string

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 30, 2014

Member

I think we may need two versions of enforceSSL and carefully check the documentation of each function for which of the two error codes is actually set. Alternatively, just querying both and displaying only the one(s) that indicate an error would be an option.

Member

s-ludwig commented Aug 30, 2014

I think we may need two versions of enforceSSL and carefully check the documentation of each function for which of the two error codes is actually set. Alternatively, just querying both and displaying only the one(s) that indicate an error would be an option.

@cifvts

This comment has been minimized.

Show comment
Hide comment
@cifvts

cifvts Aug 30, 2014

Contributor

I tried to understand the difference and to me it looks that SSL_get_error() return code is not meant to work with ERR_error_string().

Contributor

cifvts commented Aug 30, 2014

I tried to understand the difference and to me it looks that SSL_get_error() return code is not meant to work with ERR_error_string().

@s-ludwig s-ludwig added the discussion label Sep 19, 2014

@cifvts

This comment has been minimized.

Show comment
Hide comment
@cifvts

cifvts Sep 24, 2014

Contributor

I dug more into this:

  • SSL_get_error() returns a generic SSL Error but the error stays in the error queue so it may affects later calls;
  • SSL_get_error() returns the error of the SSL object which is generic error;
  • ERR_get_error() returns the correct error code for ERR_error_string() and removes that error from the error queue.

So the right approach IMHO would be to check if an error exists using SSL_get_error() and then get the error (or the errors) using ERR_get_error().

Contributor

cifvts commented Sep 24, 2014

I dug more into this:

  • SSL_get_error() returns a generic SSL Error but the error stays in the error queue so it may affects later calls;
  • SSL_get_error() returns the error of the SSL object which is generic error;
  • ERR_get_error() returns the correct error code for ERR_error_string() and removes that error from the error queue.

So the right approach IMHO would be to check if an error exists using SSL_get_error() and then get the error (or the errors) using ERR_get_error().

@cifvts

This comment has been minimized.

Show comment
Hide comment
@cifvts

cifvts Sep 26, 2014

Contributor

Trying to refactor the patch I saw there is no need to check both function, we already know an error exists and we only need to give the user a proper error string so he can understand why SSL failed. Using ERR_get_error() / ERR_error_string() we do the job.

Contributor

cifvts commented Sep 26, 2014

Trying to refactor the patch I saw there is no need to check both function, we already know an error exists and we only need to give the user a proper error string so he can understand why SSL failed. Using ERR_get_error() / ERR_error_string() we do the job.

Use ERR_get_error to get last SSL error in openssl.d
Getting last error should return an error code which  is easier to understand
when tranlated into a error string
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 27, 2014

Member

Sounds reasonable, thanks for looking up the details. I'll add an additional loop that collects and logs and additional errors in the queue and only throws the last one, just to be sure.

Member

s-ludwig commented Sep 27, 2014

Sounds reasonable, thanks for looking up the details. I'll add an additional loop that collects and logs and additional errors in the queue and only throws the last one, just to be sure.

s-ludwig added a commit that referenced this pull request Sep 27, 2014

Merge pull request #796 from cifvts/ssl_better_error
Use ERR_get_error to get last SSL error in openssl.d

@s-ludwig s-ludwig merged commit ce6eb1f into vibe-d:master Sep 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@cifvts cifvts deleted the cifvts:ssl_better_error branch Sep 27, 2014

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