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.
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
Correct EVP_PKEY ownership #535
Correct EVP_PKEY ownership #535
Changes from 7 commits
cc6b159
9a0c6b1
6b2a901
92fb4d9
f68ad36
878191a
74271f9
64a8349
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok, but for future better to set err type SOTER_INVALID_PARAMETER in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should return SOTER_FAIL in this case. EC_KEY_new_by_curve_name() could fail if the nid argument is invalid, but we’re checking for that here (
nid == 0
):themis/src/soter/boringssl/soter_asym_ka.c
Lines 41 to 45 in 74271f9
That’s why the most likely reason for failure is some internal error in cryptographic backend. SOTER_INVALID_PARAMETER is used to indicate to the user that they broke some contract for expected function parameters and this error can be avoided by passing correct parameter values. It's used for ‘obvious’ errors like using NULL values where they are not expected, while SOTER_FAIL usually indicates deeper issues with arguments, or acts like a generic catch-all error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about your coding style guide, but it would be better to swap this parameters for better reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it can be rewrited with less code using EVP_PKEY_assign_EC_KEY(). I don't see any reason to use EVP_PKEY_set1_EC_KEY() in this plase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the Yoda-style comparisons, they are not prescribed by the coding style and we don’t really like them that much (exactly for readability reasons). But it’s tedious to walk through all instances of such legacy code at once, so we incrementally modernize it during refactoring. Thank you for noting 👍 This code style, much readable it is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re absolutely right about EVP_PKEY_assign_EC_KEY(), It avoid unnecessary reference counting. I’ll update this part. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I remember, this type of comparisons was used to avoid errors in C code like
if (a = someFunc())
. Using constant values (rvalue) as left operand of comparison you will see error on compilation because can't assign value if miss one "=" sign. It's less readable but saves from such errorsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another merit of this style is that it actually makes comparisons with long function calls more readable if they are made inline. Compare
with
where it's really easy to miss whether it’s
==
or!=
and if it’s-1
, or0
, or whatever.Though, it’s less of an issue with our current code style that places a reasonable limit on the line length:
or even better:
As for accidentally using
=
instead of==
, I personally favor the-Wparentheses
compiler warnings over an unnatural code style. So I’m all in for making the dog wag the tail, just without having a dedicated 8000-line-change that does only that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how this flag will save from assigning in "if" statements which allowed by language syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lagovas, a typical mistake looks like this:
in which case the compiler will give a friendly warning:
This has to be addressed, given that we treat warnings as errors in pull requests.
Obviously, if you do intent to make an assignment in conditional then you may explicitly code it like this (note the double parentheses):
(It works for boolean function arguments as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this plase we get dereferencing a pointer. It would be good to check ctx before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is not in SOTER_PRIVATE_API and i think that it would be good make that check. Same in soter_sign_final_ecdsa_none_pkcs8 (in this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions that are not marked SOTER_API are considered to be private to Soter and they can make assumptions about arguments. Usually we check arguments for NULL in public functions and let the private ones assume that the arguments are more or less sane.
In this case soter_sign_init_ecdsa_none_pkcs8() is called by soter_sign_init() which does check that ctx is non-NULL beforehand. It’s also checked a couple of times more up the call stack, so we can be sure that it won’t be NULL in soter_sign_init_ecdsa_none_pkcs8().
SOTER_PRIVATE_API is kind of a hack that intentionally exposes private API for Themis. It’s not really pretty, but some code currently calls such functions and makes necessary precautions to ensure that they are safe to call. At some point we intend to get rid of SOTER_PRIVATE_API to avoid confusion over what’s public and what’s private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanation. Now I have knowledge how this visibility works. This kind of bug is not connected with PR. But you have such functions in SOTER_API. Ex. is soter_rsa_key_pair_gen_init(). This is notice to be cerfull with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mb it will be good to make some rule how to work with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, with soter_rsa_key_pair_gen_init() is bad example (becouse it should be not in SOTER_API). But if we look at typtcal usage any public funtion it would be checked twice ore even more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, C does not really provide any amenities for null checks. Theoretically, we could use
__attribute__((nonnull))
or something like that, but that’s not really portable and too much of a bother. Generally, the rules are that public functions (marked with SOTER_API or THEMIS_API) have to doparanoidthorough parameter checking while internal functions may expect the parameters to have reasonable values. And generally, it does not really hurt if internal functions have duplicated checks. Better safe than segfault.