Added the missing safety checks and support for double, @paulidale :)

Perhaps there is scope to meet in the middle between this and #8320

I think I'd be happy with sized integers (only int32_t, int64_t, uint32_t, uint64_t), BN and double. Plus integer widening from thirty two bit to sixty four bit (I don't see narrowing being as useful -- when was the last time you thought drat, I wish I'd used a shorter integer).

I'll prototype something which does this using casts/type conversion rather than the memcpys.

Middle ground in #8379

I see where you'd going here... however, there's one thing that I find real troubling with this proposal, and it's that the size of an integer can now be determined in two different ways, 1) from the data type, and 2) from the buffer size. What happens if they don't match?

If they don't match, it's a programming error. It's the equivalent of doing this:

short s = ...someval...;
int ip = (int *)&s;
int i = *ip;

If you use the published API and macros it won't happen.
Try this code:

; cat a.c ; gcc a.c ; ./a.out

#include <stddef.h>
#include <stdio.h>
int main() {
    printf("int %d long %d float %d pointer %d\n",
	    (int)sizeof(int),
	    (int)sizeof(long),
	    (int)sizeof(double),
	    (int)sizeof(char *));
    return 0;
}
int 4 long 8 float 8 pointer 8

Please look at commit 1b6af2041d866dac5b8df2742c497373e80f55ba which handles BIGNUM, in particular the comments. Is this okay?

I remain doubtful about the need for several integer data types for size purposes. If we were talking differences in how the bits are organized, it would make much more sense.
(for example, reals have a different organization, so it makes complete sense to have a different data type for them)

I think the PARAM should exactly match the API. If an API takes an int allowing the user to put in a variable-length integer seems wrong. It also adds a new source of run-time errors. Finally, sacrificing clarity and efficiency for ... what, some architectural purity? Not sure what to call it; I use the term "geek esthetics."

It also adds a new source of run-time errors.

We do that either way.

We do this anyway

NO. It does not. If the native API takes an int, putting anything other than an int fails at compile-time.

So you say BN_set should put the size used in return_size, and BN_get should use that field?

I moved much of the return_size setting. It is not clear to me what return_size means for a bignum. Is it the number of bytes used in the serialization buffer? Or is it the number of bytes in the bignum? The current serialization cannot handle negative numbers so making both numbers always be the same seems wrong.

For big numbers, the return size is supposed to be the space it's serialization takes up in the buffer. BN_num_bytes() gives you the answer for OpenSSL bignums.

Regarding the sign, we can probably get away with only supporting unsigned numbers.

We do this anyway

NO. It does not. If the native API takes an int, putting anything other than an int fails at compile-time.

Huh?

You are setting an arbitrary number (data_type) and having a void pointer (buffer) point at the data.

Please do tell how your numbers provide better type security.

Let me turn that question back to you. If everything is a multi-byte number, you have a run-time error when someone puts too big a value there. In C with a header file in scope of course, this doesn't happen, putting a long/int64_t/size_t generates a warning. We don't have that situation here. But when using the macro's and the public get/set int API, you can only put an INT there

Then the documentation should say that only unsigned BN's are supported and that BN_num_bytes only works for non-negative values. It is inconsistent that we are supporting all sorts of integer types that openssl itself does not use, but you're willing to live with only partial BN support.

But when using the macro's and the public get/set int API

That's a condition for your claim.

you can only put an INT there

And the numbers in the OSSL_PARAM are still just numbers. In the base of base, i.e. the raw OSSL_PARAM, you have traded the data_type + buffer_size combo for a data_type-and-let's-hope-buffer_size-matches.

An API that helps make things safer is absolutely fine, I have nothing against that, and as long as all parts of the API are consistent with each other, that's absolutely fine.

However, the claim that exchanging a set of numbers in the underlying data type for another set of numbers would somehow enhance C type safety is simply false. But that doesn't change the added safety in an API added on type, regardless of the numbers. Please see the difference.

I didn't swap "one number for another." I have a type and a size. Both are necessary as I tried to show in #8377 (comment)

I didn't swap "one number for another." I have a type and a size. Both are necessary as I tried to show in #8377 (comment)

I'm done arguing this point. We're obviously each in our personal trench, I see no reason continuing.

Speaking of something different, have you noticed this:

https://travis-ci.org/openssl/openssl/jobs/501671755#L3785-L3807

ccache gcc  -Iinclude -Iapps/include  -pthread -m64 -Wall -O3 -DDEBUG_UNUSED -DPEDANTIC -pedantic -Wno-long-long -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wswitch -Wsign-compare -Wshadow -Wformat -Wtype-limits -Wundef -Werror -Wmissing-prototypes -Wstrict-prototypes -DNDEBUG -D_DEFAULT_SOURCE -ansi  -c -o test/params_api_test-bin-params_api_test.o test/params_api_test.c
test/params_api_test.c: In function 'test_params':
test/params_api_test.c:31:9: error: initializer element is not computable at load time [-Werror]
         OSSL_PARAM_uint64("u64param", &u64),
         ^
test/params_api_test.c:31:9: error: initializer element is not computable at load time [-Werror]
test/params_api_test.c:32:9: error: initializer element is not computable at load time [-Werror]
         OSSL_PARAM_int("intparam", &i),
         ^
test/params_api_test.c:32:9: error: initializer element is not computable at load time [-Werror]
test/params_api_test.c:33:9: error: initializer element is not computable at load time [-Werror]
         OSSL_PARAM_size_t("--unused--", &sz),
         ^
test/params_api_test.c:33:9: error: initializer element is not computable at load time [-Werror]
test/params_api_test.c:34:9: error: initializer element is not computable at load time [-Werror]
         OSSL_PARAM_double("doubleparam", &d1),
         ^
test/params_api_test.c:34:9: error: initializer element is not computable at load time [-Werror]
test/params_api_test.c:35:9: error: initializer element is not computable at load time [-Werror]
         OSSL_PARAM_bignum("bignumparam", buffer, sizeof(buffer)),
         ^
test/params_api_test.c:35:9: error: initializer element is not computable at load time [-Werror]
cc1: all warnings being treated as errors

It seems to only happen when building with -ansi

Travis error fixed

I changed locate to not set an error. This caused some const complaints, not thrilled with the cast but it's the least invasive option I think.

On the discussion on type safety I think there is some confusion here about what can be provided by this type of an API.

We can think about compile time type safety and run time type safety. We're not going to get compile time type safety because ultimately everything gets converted to void * pointers and there are casts. That's a fundamental property of the OSSL_PARAM structure, and its an accepted design decision.

What we can do is put run time layers around the underlying structure to give us back some of that safety at run time. I guess that's what this PR and #8451 are all about.

So when we think about run time type safety we have to consider what is the format and size of the data as it is stored in an OSSL_PARAM, and is it consistent with the type of the data that is expected. A run time type safety check would validate that the two are consistent, e.g. if we were expecting an unsigned 32-bit integer, then we need to make sure that the value stored in the OSSL_PARAM isn't negative and it is less than or equal to 32-bits in length. As long as that is true then our run time type safety check has passed.

It makes no difference to the run time type safety check how that data is physically stored in the OSSL_PARAM. As long as we know the format of the underlying data and its size then that is enough. If we are expecting an unsigned 32 bit integer, and the data we've got is an unsigned integer and it fits in less than (or equal) to 32-bits then we're good.

It seems to me that both this PR and #8451 achieve that goal. For example, both have the ability to take a size_t and encode it into an OSSL_PARAM structure, and both have the ability to decode a size_t from an OSSL_PARAM structure and complain if the format/size isn't right.

I don't see a particular benefit of this PR over #8451 in terms of type safety. In fact I think this PR confuses the types of what we are expecting to send/receive with how those types are encoded within an OSSL_PARAM structure. So I think we should proceed with #8451 over this PR.

In my mind neither of the PRs fully address defining what types we are expecting to send/receive in any particular scenario. But I think that aspect can be covered in a follow on PR. I've written a little of my thoughts on that aspect in issue #8454.

Thank you for the thoughtful review. Have you read the revised manpage? I believe this is now also easier to use, and a more natural fit for C programmers. I will finish up my implementation, anyway.

Have you read the revised manpage?

Yes.

Github apparently won't let me reply inline to:

I think we have to have a clear format and not just say "whatever the format of BIGNUM bn2bin is" and definitely not a BIGNUM directly. That format of bn2bin is (presumably) defined somewhere so this shouldn't be hard. We don't want to require Provider authors to have to link to libcrypto, so if they want to use gmp instead, then that's fine. Yes it should preserve the sign.

so I'll note that any sort of public-visible "BIGNUM-equivalent" thing seems like it's going to necessitate exposing BN_ULONG, which I somehow thought we were trying to move away from.

so I'll note that any sort of public-visible "BIGNUM-equivalent" thing seems like it's going to necessitate exposing BN_ULONG, which I somehow thought we were trying to move away from.

Um, no? It's just an array of bytes?

Since #8451 is now merged, this PR serves no direct purpose. Closing (in agreement with @richsalz)