WIP: Allocate param array entries dynamically.#9254
WIP: Allocate param array entries dynamically.#9254paulidale wants to merge 3 commits intoopenssl:masterfrom
Conversation
|
This is intended to provide a way for OSSL_PARAM structures to escape a stack frame more easily. Tests, documentation, string types, BN and bug fixing are still required. |
|
I prefer this over #9246. params[foo_pos = n++] = OSSL_PARAM_allocate_int("foo", 42);... or: foo_pos = n;
params[n++] = OSSL_PARAM_allocate_int("foo", 42); |
|
My intention is to have a flag bit (or type bit) to indicate allocated. Because the function must return an OSSL_PARAM structure, error detection needs to occur later -- I've included a function for this here. Likewise, a free function would be desirable. |
|
What you're implying here as well as in #9139 is that the creator of the OSSL_PARAM array looses control of that array, i.e stops being the owner. I think that's a bad direction to follow, and adds a ton of complexity. |
|
I have a different approach to this in the works. At the moment it is EVP specific (because I wrote it for a very specific problem) but it could easily be made generic. It's not yet ready for prime time so I've not made a PR from it yet. See: The idea is to create a temporary "template" which then gets converted to an OSSL_PARAM at the end in a single OPENSSL_zalloc call (or OPENSSL_secure_zalloc as appropriate). The whole OSSL_PARAM structure can then be passed around as required, and the whole thing can be freed with a single OPENSSL_free call at the end. |
|
The creator doesn't lose control in either. Here, the creator has complete control -- populate the OSSL_PARAM array, pass it to the consumer, free it. In #9246 the creator needs to keep control, the creator allocates the memory and must free it. Call the creator to get the param array, populate it, call the creator to use it and either call again to free it or have the use free as well. |
|
@mattcaswell, that's kind of heading in a similar direction to #9246 with its single allocation/free. I used a varargs function rather than building the description up piecemeal but the intent seems similar. |
|
Note that whatever solution is eventually chosen, it will need to be able to handle secure malloc so that we can pass private data as an OSSL_PARAM. |
|
The template approach mentioned in #9254 (comment) is what I did for #8377; defining the parameters was a separate step from allocating storage. |
This provides an alternative for the OSSL_PARAM_construct_ series of functions which allocate storage for the values rather than relying on local on stack storage. The new OSSL_PARAM_allocate_ and the existing OSSL_PARAM_construct_ calls can be freely intermingled.
fb15f59 to
d21b545
Compare
richsalz
left a comment
There was a problem hiding this comment.
I read through this, but only looked at the secure BN stuff (it's too hairy and complicated for me to follow, sorry). The secure BN part seems okay.
|
|
||
| if (p != NULL) { | ||
| if (data == NULL) | ||
| memset(p, 0, data_size); |
There was a problem hiding this comment.
use ?: instead of the nested if.
crypto/params.c
Outdated
| if (bsize == 0) | ||
| bsize = (size_t)BN_num_bytes(bn); | ||
|
|
||
| if (BN_get_flags(bn, BN_FLG_SECURE)) { |
There was a problem hiding this comment.
Add == BN_FLG_SECURE
Also, this function needs comments.
crypto/params.c
Outdated
| flag |= OWNER_FLAG_secure; | ||
| } | ||
| } | ||
| if (secure) |
There was a problem hiding this comment.
I prefer the ?: operator, IMO
This provides an alternative for the
OSSL_PARAM_construct_series of functions which allocate storage for the values rather than relying on local on stack storage. The newOSSL_PARAM_allocate_and the existingOSSL_PARAM_construct_calls can be freely intermingled.