Cleanup ossl_*_new() functions #912
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, calling ossl_bn_new() with a NULL argument allocates a new OpenSSL::BN instance representing 0. This behavior is confusing. Raise an exception if this is attempted, instead.
These functions are not actually called with NULL. It also doesn't make sense to do so, so let's simplify the definitions.
These functions are not actually called with NULL.
Similar to most of the other ossl_*_new() functions, let it take a const pointer and make a copy of the object. This also fixes a potential memory leak when the wrapper object allocation fails.
Likewise, let it take a const pointer and not the ownership of the OpenSSL object. This fixes potential memory leak in OpenSSL::OCSP::BasicResponse#status.
Among functions named ossl_*_new(), ossl_pkey_new() is now the only one that takes ownership of the passed OpenSSL object instead of making a copy or incrementing its reference counter. Rename it to make this behavior easier to understand.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ossl_*_new()functions are confusingly inconsistent in how they handle the lifetime of the passed OpenSSL object. This PR makes the behavior consistent, remove dead code, and fix potential memory leaks due to allocating Ruby objects after allocating OpenSSL objects.bn: avoid ossl_bn_new(NULL)
Currently, calling ossl_bn_new() with a NULL argument allocates a new
OpenSSL::BN instance representing 0. This behavior is confusing. Raise
an exception if this is attempted, instead.
x509: disallow ossl_x509{,attr,crl,ext,revoked,name}*_new(NULL)
These functions are not actually called with NULL. It also doesn't make
sense to do so, so let's simplify the definitions.
pkcs7: disallow ossl_pkcs7{si,ri}_new(NULL)
These functions are not actually called with NULL.
ocsp: refactor ossl_ocspsres_new()
Similar to most of the other ossl_*_new() functions, let it take a const
pointer and make a copy of the object.
This also fixes a potential memory leak when the wrapper object
allocation fails.
ocsp: refactor ossl_ocspcertid_new()
Likewise, let it take a const pointer and not the ownership of the
OpenSSL object.
This fixes potential memory leak in OpenSSL::OCSP::BasicResponse#status.
pkey: rename ossl_pkey_new() to ossl_pkey_wrap()
Among functions named ossl_*_new(), ossl_pkey_new() is now the only one
that takes ownership of the passed OpenSSL object instead of making a
copy or incrementing its reference counter. Rename it to make this
behavior easier to understand.