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

param: Param add a modified indicator to OSSL_PARAM. #11588

Closed
wants to merge 1 commit into from

Conversation

@paulidale
Copy link
Contributor

@paulidale paulidale commented Apr 21, 2020

Using the return_size field as a sentinel for change detection.

  • documentation is added or updated
  • tests are added or updated
@paulidale paulidale added this to the 3.0.0 milestone Apr 21, 2020
@paulidale paulidale added this to In progress in 3.0 New Core + FIPS via automation Apr 21, 2020
@paulidale paulidale changed the title Param used param: Param add a modified indicator to OSSL_PARAM. Apr 21, 2020
doc/man3/OSSL_PARAM_int.pod Outdated Show resolved Hide resolved
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Apr 21, 2020

I wonder if it would be better to not document how the modification detection works.

doc/man3/OSSL_PARAM_int.pod Outdated Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Reviewer approved Apr 21, 2020
@levitte
Copy link
Member

@levitte levitte commented Apr 21, 2020

I'm wondering if you shouldn't mention what the value of OSSL_PARAM_UNMODIFIED is, and declare it "reserved". If nothing else, those who worry that their particular huge buffer size might have the same value will feel informed.

@levitte
Copy link
Member

@levitte levitte commented Apr 21, 2020

I wonder if it would be better to not document how the modification detection works.

Why?

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Apr 21, 2020

Why?

To give us some flexibility to change it without incurring breakage. I can't guarantee that it will never need changing. By not spelling it out, we can change it because the helper functions and macros hide the details.

@romen
Copy link
Member

@romen romen commented Apr 21, 2020

Why?

To give us some flexibility to change it without incurring breakage. I can't guarantee that it will never need changing. By not spelling it out, we can change it because the helper functions and macros hide the details.

Would it be more future proof to have a separate flag variable to track the state of an OSSL_PARAM?

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Apr 21, 2020

Would it be more future proof to have a separate flag variable to track the state of an OSSL_PARAM?

Yes, but it was shot down in the past.

Stealing a bit from the type field would be another possibility (although this would be far too invasive to consider now).

OSSL_PARAM is a public structure and we want to keep it plain and simple. A version number and some flags wouldn't go astray but...

@levitte
Copy link
Member

@levitte levitte commented Apr 21, 2020

OSSL_PARAM is a public structure

... and that's exactly the reason to document the unmodified trick

Copy link
Contributor

@slontis slontis left a comment

Should this be also cleaning up where we already use the return_size in this fashion. There are quite a few of them.

@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Apr 21, 2020

Should this be also cleaning up where we already use the return_size in this fashion. There are quite a few of them.

We should clean them up. I'd prefer to get this in and to then address the other uses. I didn't release that such uses were widespread.

@levitte
Copy link
Member

@levitte levitte commented Apr 21, 2020

I didn't release that such uses were widespread.

They aren't yet.

: ; git grep 'return_size =.*not_set'
crypto/evp/p_lib.c:    params[0].return_size = not_set;
crypto/evp/p_lib.c:        if (params[0].return_size == not_set
crypto/evp/p_lib.c:    if (params[0].return_size == not_set)
crypto/evp/p_lib.c:    params[0].return_size = not_set;
crypto/evp/p_lib.c:    if (params[0].return_size == not_set)
crypto/evp/p_lib.c:    params[0].return_size = not_set;
crypto/evp/p_lib.c:    if (params[0].return_size == not_set)
crypto/evp/p_lib.c:    params[0].return_size = not_set;
crypto/evp/p_lib.c:    if (params[0].return_size == not_set)
crypto/evp/p_lib.c:    params[0].return_size = not_set;
crypto/evp/p_lib.c:    if (params[0].return_size == not_set)

void OSSL_PARAM_set_unmodified(OSSL_PARAM *p)
{
if (p != NULL)

This comment has been minimized.

@richsalz

richsalz Apr 21, 2020
Contributor

dislike the NULL checks here and above FWIW.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Apr 21, 2020

Since the structure is public, and since the define's are public, and since openssl considers the contents of the header files, and not just the documentation of them, to be part of the API/ABI, might as well document what things currently do.

@paulidale paulidale force-pushed the paulidale:param-used branch to ee85ff2 Apr 22, 2020
@paulidale
Copy link
Contributor Author

@paulidale paulidale commented Apr 22, 2020

Merged to master, thanks.

@paulidale paulidale closed this Apr 22, 2020
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Apr 22, 2020
openssl-machine pushed a commit that referenced this pull request Apr 22, 2020
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #11588)
@paulidale paulidale deleted the paulidale:param-used branch Apr 22, 2020
@kaduk
Copy link
Contributor

@kaduk kaduk commented Apr 22, 2020

since openssl considers the contents of the header files, and not just the documentation of them, to be part of the API/ABI

I thought I remembered the stance being a bit more subtle than that; do you have a pointer handy ("no" is a fine answer)?

@richsalz
Copy link
Contributor

@richsalz richsalz commented Apr 22, 2020

Sorry, I don't. Perhaps @t-j-h might reply here, since he was an advocate of that view.

@levitte
Copy link
Member

@levitte levitte commented Apr 22, 2020

Generally speaking, we consider the public headers to define the API. However, I would argue that some of those headers or APIs have become public by accident, so it's possible to justify that some of them should stop (see parts of the CONF API that was recently made unpublic).

OSSL_PARAM is intentionally a public structure, however, and very much part of the libcrypto<->provider interface, so it's safe to say that it should be considered part of the public API.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Apr 22, 2020

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

Successfully merging this pull request may close these issues.

None yet

7 participants