Reordering new API's that have a libctx, propq

[Nicola Tuveri]

On Sat, Sep 5, 2020, 14:01 Tim Hudson <tjh at cryptsoft.com> wrote:

> On Sat, Sep 5, 2020 at 8:45 PM Nicola Tuveri <nic.tuv at gmail.com> wrote:
>
>> Or is your point that we are writing in C, all the arguments are
>> positional, none is ever really optional, there is no difference between
>> passing a `(void*) NULL` or a valid `(TYPE*) ptr` as the value of a `TYPE
>> *` argument, so "importance" is the only remaining sorting criteria, hence
>> (libctx, propq) are always the most important and should go to the
>> beginning of the args list (with the exception of the `self/this` kind of
>> argument that always goes first)?
>>
>
> That's a reasonable way to express things.
>
> The actual underlying point I am making is that we should have a rule in
> place that is documented and that works within the programming language we
> are using and that over time doesn't turn into a mess.
> We do add parameters (in new function names) and we do like to keep the
> order of the old function - and ending up with a pile of things in the
> "middle" is in my view is one of the messes that we should be avoiding.
>

We are already adding new functions, with the ex suffix, to allow users to
keep using the old version, so given that the users passing to the "_ex"
function are already altering their source, why are we limiting us from
rationalizing the signature from the PoV of new users and old users alike
that are in general quite critic of our API usability, even if it means
that applying both "required-ness" and "importance" as sorting criteria
sometime we end up adding in the middle?

I don't think I am being dismissive of the needs of existing applications
here: if a maintainer is altering their code from using "EVP_foo()" to
"EVP_foo_ex()", they will likely also be looking at the documentation of
the old and the new API versions and there shouldn't be really any
additional significanf cost in adding a parameter a the edges or in the
middle.

I think the importance argument is the one that helps for setting order and
> on your "required args" coming first approach the importance argument also
> applies because it is actually a required argument simply with an implied
> default - which again puts it not at the end of the argument list. The
> library context is required - always - there is just a default - and that
> parameter must be present because we are working in C.
>

I think I disagree with this, from the API CONSUMER PoV there is a clear
difference between a function where they don't need to pass a valid libctx
pointer and instead pass NULL (because there is a default associated with
passing NULL) and a function like an hypothetical
OSSL_LIBCTX_get_this(libctx) or OSSL_LIBCTX_set_that(libctx, value).

In the first case the function operates despite the programmer not
providing an explicit libctx (because a default one is used), in the latter
the programmer is querying/altering directly the libctx and one must be
provided.

*… actually only now that I wrote out the greyed text above I think I am
starting to understand what you meant all along!*

Your point is that any API that uses a `libctx` (and `propq`) is always
querying/altering a libctx, even if we use NULL as a shortcut to mean "the
global one", so if we are fetching an algorithm, getting/setting something
on a libctx scope, creating a new object, we are always doing so from a
given libctx.
In that sense, when an API consumer is passing NULL they are not passing
"nothing" (on which my "optional arguments" approach is based), but they
are passing NULL as a valid reference to a very specific libctx.

I now see what you meant, and I can see how it is a relevant thing to make
evident also in the function signature/usage.

But I guess we can also agree that passing NULL as a very specific
reference instead of as "nothing", can create a lot of confusion for
external consumers of the API, if it took this long for one of us — ­*ok,
it's me, so probably everyone else understood your point immediately, but
still…* — to understand the difference you were pointing out, even knowing
how these functions work internally and being somewhat familiar with
dealing with libctx-s.
If we want to convey this difference properly to our users, would it be
better to use a new define?

#define OSSL_DEFAULT ((void*)42)

- OSSL_DFLT could be a shorter alternative, if we prefer brevity.
- I am intentionally not specifying _LIBCTX as we might reuse a similar
mechanism to avoid overloading NULL in other similar situations: e.g. for
propq, where NULL is again a shortcut for the global ones, compared with
passing "" as an empty properties query, seems like another good candidate
for using a symbol to explicitly highlight that the default propq will be
used
- I would avoid making OSSL_DFLT an alias for NULL to prevent users from
sidestepping the define completely and use NULL directly (because "it just
works anyway"): that is why I am picking 42 as an example, but any magic
number different from NULL/0 and that is most likely/always treated as an
invalid userspace memory address would be ok.

If we had such a define, from the API consumer PoV, libctx stops being an
"optional argument" (in the sense I can give it "NULL/nothing" and the
function still works) and it is clearly a required argument (passing
"NULL/nothing" segfaults/errors) while still retaining the option of having
a shortcut symbol to delegate the task of retrieving the default libctx to
the called function.

Whatever it is that we end up decided to do, the rule should be captured
> and should also allow for the fact that we will evolve APIs and create _ex
> versions and those two will also evolve and a general rule should be one
> that doesn't result in an inconsistent treatment for argument order as we
> add _ex versions.
>

If we were not using NULL as an alias for "the default one" rather than for
"nothing", the difference between required and simil-optional arguments
(those that you can avoid passing to the function by passing
"NULL/nothing") would be evident to the users of the API.

In that case we could agree on sorting arguments as:
- required ones (you must pass a valid value, including the special
OSSL_DFLT to let the callee retrieve a default value dynamically, or
passing NULL/nothing as the output buffer to obtain a length for
preallocation)
- simil-optional ones (you can pass "NULL/nothing": the function will
perform its operations skipping an optional part of it, or using whatever
representation of "nothing" as the value of an optional thing, or creating
a temporary thing to solve the task at hand in the stead of the one the
caller did not pass, etc.)
- callback+args

and sorting by "importance" within each group:
- among required args:
  * the equivalent of "self/this" for the functions that operate as object
methods in a OOP fashion
  * libctx (for OOP-like functions, if an object is not libctx-agnostic,
there should be very few occasions where both self and libctx appear as
arguments, as the libctx should be embedded in the object at creation time
if it depends from it, i.e. let's not repeat the EC_POINT/EC_GROUP mess!)
  * propquery (always right after libctx, see Note1 below about
libctx+propquery)
  * functionality-specific required args, in their order of importance (we
need to further formalize this, e.g. we have not always been consistent on
the order of inputs and outputs, though most of the recommended API are
quite consistent in having outputs first, then inputs)
- among optional args:
  * functionality-specific optional args, in their order of importance
  * optional args for auxiliary objects (I would define these as openssl
specific things that aid in solving a task, but are not required to solve a
task, e.g. BN_CTX, see Note2 below))

*Note1*: I am listing propquery separately, although there should be very
few cases where we deliberately have libctx as an argument of a function,
and no propquery alongside it. If we are querying/altering a libctx, there
is a very high chance that we require propquery to fetch something from it;
the fact that, in our *current* implementation/plans, a function taking
libctx as an argument does not use/support propquery for accomplishing its
task, does not exclude that in the future part of that task might involve
fetching another thing from the passed libctx. Given that once it's out in
a release that function will stay with us for years to come, we should make
extra sure we have a very solid argument covering the current and
foreseeable future of that function, even at the cost of having a propquery
argument ignored by the current implementation of that function, and
marking it down as unused but reserved for future use in the documentation.
Note that we currently have functions that take both libctx and propq, but
they are not one after the other, eg. `EVP_PKEY_CTX_new_from_pkey(libctx,
pkey, propq)`, if we agree that a libctx argument should always come with
an attached porpq argument, there is no reason to separate them

*Note2*: (expanding on BN_CTX as an optional auxiliary object) In the
lifetime of a single high level RSA/DH/DSA/EC operation we have performance
benefits from avoiding frequent heap (re)allocations, so we have this
openssl specific object as a short-lived stack-like cache, interposing
between malloc/realloc/free and the various BN_this() and BN_that() that
compute the result of the high level operation. BN_CTX cannot be a global
(or libctx-scoped) object, as we want to retain separation between
unrelated high level operations and limit the surface for potential
side-channel leakage. As a result most of the `BN_` functions that do not
operate in-place have an optional `BN_CTX *` argument: the function will
use the provided BN_CTX if it is passed, or create a temporary one if
"nothing/NULL" is passed. Strictly speaking this BN_CTX object is not a
functional requirement, as the implementation could easily replace all the
`BN_CTX_get()` calls with conditional usage of either `BN_new()` or
`BN_CTX_get()` and with corresponding conditional `BN_free()`, it is our
choice to default to creating a fresh `BN_CTX` if the caller passes NULL,
to improve code readability and maintainability. From the API consumer PoV,
typing `BN_mul(r, a, b, NULL)` means "compute `r = a * b`, with no
preexisting BN_CTX" and the implementation might (or might not) create a
temporary BN_CTX to use internally or avoid the BN_CTX mechanism altogether.

What do you (all) think about it?

Nicola

>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mta.openssl.org/pipermail/openssl-project/attachments/20200905/5f55a775/attachment-0001.html>