Ping @openssl/committers ?

A second review is required.

your colleague @slontis approved. :) maybe he wants to do this as his first official commit?

We are updating the tooling to make sure same-organisation approvals aren't seen as a match to avoid any potential perceived conflict of interest in the review process ...

@t-j-h nice to see that being made policy (since i first started it about akamai). Just for clarification, does that also mean not approving PR's from one's one organization?

In future I will not approve if @paulidale has already approved. (This applies to our own PR's also).

Rebased; needs reviews.

Needs a rebase and a second review. ignore this, stale page.

Has a decision about the control functions been made about them being offered from providers or not?

I will continue to point out that set_params and get_params type functions can be used as controls. It's all in what the receiving end does in response to OSSL_PARAM values...

Ping @openssl/committers ?

Regarding dispatch macro names, @richsalz and I have already had a discussion in #8887 and disagree somewhat. At the very least, I think that _GET_ in the middle of them unnecessary.

Also, for naming purposes, I'm thinking that if we do want to have something that is explicitly meant to provide the public OpenSSL API, the dispatch macro names for them should maybe have a different prefix than OSSL_FUNC_CORE... perhaps OSSL_FUNC_OSSL? So, something like this?

#define OSSL_FUNC_OSSL_CRYPTO_MALLOC       nn
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_malloc, (size_t num, const char *file, int line))

Maybe @levitte & @mattcaswell should confer on these names so it can move forward.

We had a quick chat, and @mattcaswell said something that reminded me to look more closely in core_numbers.h and remind myself.
There's this comment:

/*-
 * Identities
 * ----------
 *
 * All series start with 1, to allow 0 to be an array terminator.
 * For any FUNC identity, we also provide a function signature typedef
 * and a static inline function to extract a function pointer from a
 * OSSL_DISPATCH element in a type safe manner.
 *
 * Names:
 * for any function base name 'foo' (uppercase form 'FOO'), we will have
 * the following:
 * - a macro for the identity with the name OSSL_FUNC_'FOO' or derivates
 *   thereof (to be specified further down)
 * - a function signature typedef with the name OSSL_'foo'_fn
 * - a function pointer extractor function with the name OSSL_'foo'
 */

Following that, and if the intention is to have a 1:1 mapping to the public API, then it should look like this, right?

/* Memory allocation, freeing, clearing. */
#define OSSL_FUNC_CRYPTO_MALLOC       10
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_malloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_ZALLOC       11
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_zalloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_MEMDUP       12
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_memdup, (const void *str, size_t siz, const char *file, int line))
#define OSSL_FUNC_CRYPTO_STRDUP       13
OSSL_CORE_MAKE_FUNC(char *,
        CRYPTO_strdup, (const char *str, const char *file, int line))
#define OSSL_FUNC_CRYPTO_STRNDUP      14
OSSL_CORE_MAKE_FUNC(char *,
        CRYPTO_strndup, (const char *str, size_t s, const char *file, int line))
#define OSSL_FUNC_CRYPTO_FREE         15
OSSL_CORE_MAKE_FUNC(void,
        CRYPTO_free, (void *ptr, const char *file, int line))
#define OSSL_FUNC_CRYPTO_CLEAR_FREE   16
OSSL_CORE_MAKE_FUNC(void,
        CRYPTO_clear_free, (void *ptr, size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_REALLOC      17
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_realloc, (void *addr, size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_CLEAR_REALLOC 18
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_clear_realloc, (void *addr, size_t old_num, size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_MALLOC 19
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_secure_malloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_ZALLOC 20
OSSL_CORE_MAKE_FUNC(void *,
        CRYPTO_secure_zalloc, (size_t num, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_FREE   21
OSSL_CORE_MAKE_FUNC(void,
        CRYPTO_secure_free, (void *ptr, const char *file, int line))
#define OSSL_FUNC_CRYPTO_SECURE_CLEAR_FREE 22
OSSL_CORE_MAKE_FUNC(void,
        CRYPTO_secure_clear_free, (void *ptr, size_t num, const char *file, int line))
#define OSSL_FUNC_OPENSSL_CLEANSE 23
OSSL_CORE_MAKE_FUNC(void,
        OPENSSL_cleanse, (void *ptr, size_t len))
#define OSSL_FUNC_OPENSSL_HEXSTR2BUF 24
OSSL_CORE_MAKE_FUNC(unsigned char *,
        OPENSSL_hexstr2buf, (const char *str, long *len))

(previous comment edited, I had pasted in much more than I intended...)

@levitte regarding #8886 (comment) isn't that what this PR has? But on the other hand, using one person's code comment to justify a policy seems a little weak to me :)

Re #8886 (comment), perhaps the OMC, the committers, and/or the FIPS sponsors should be included in any such discussions as how to name things? Not just two paid fellows.

I fixed the define names thanks for the fix!

Can I suggest that maybe in the future we don't need to line up the values with things like this, lots of other lines between defines? :) Just put name space value

The function and define names were fixed, this is rebased, so now it needs a reconfirm and another review.

CI failures look relevant.

Oops, clobbered some of my fixes. New commit pushed that fixes the CI (and function/macro naming) issues. There is now a new (minor?) issue, however, that is described in db3480e

argh, rebased (that was a hard one) and pushed. ping.

I cannot figure out why some of the CI builds are failing; time-out issues? Any clues?

The one I looked at is the evp_extra test failing -- I don't think this one isn't timing critical.
Hmmm...

I also was looking at that same test..
This is probably not helpful...
See if you can reproduce it locally with your changes.. (Maybe it only happens with non debug?)

export LD_LIBRARY_PATH=.
export OPENSSL_MODULES=providers
./test/evp_extra_test

If you know which test it is you can do this

./test/evp_extra_test -test 13 -iter 4

Hmm maybe one of your function signatures is wrong.. If you call a function with incorrect args it can just crash. I had an incorrect return type causing a crash a while ago.

AARGH. I figured it out. Line 234:

	    OPENSSL_CTX *ctx = OPENSSL_CTX_new();

which calls CRYPTO_zalloc, but the upcall hasn't been created yet!

Pinging @mattcaswell and anyone else who has thoughts about how to fix this issue.

This is a recent change that moved the CTX_new() to before the resolving takes place..
At least some of the resolve will need to move up before the new(). If there are any failures then maybe the statics should all be set back to NULL.

I can fix the segv, but then there's memory leak that doesn't happen on master. :( see 5a3e30c

Doing all the resolves first is reasonable, they are just looking up and setting static globals.

This is nearly two months old and got first approval more than a month ago. Since then I've rebased multiple times, worked around errors in master, etc. Right now there's an appveyor build failure which I don't understand: https://ci.appveyor.com/project/openssl/openssl/builds/25709908/job/r85f0x7jbnf6824v#L5017

This builds. It's needed. It's been sitting for weeks/months. Ping.

I'll leave the last two comments for @mattcaswell (& perhaps @levitte) to respond to.
In the meantime, I've merged this. Thanks for your patience.