Extra level of indirection for string _PTR param types removed. #9313

paulidale · 2019-07-05T01:11:19Z

The two _PTR string types have had the indirection removed. The pointer to
their data lives inside the OSSL_PARAM structure without needing an intermediate
pointer.

documentation is added or updated
tests are added or updated

The two _PTR string types have had the indirection removed. The pointer to their data lives inside the OSSL_PARAM structure without needing an intermediate pointer.

doc/man3/OSSL_PARAM.pod

levitte · 2019-07-05T04:57:13Z

test/params_test.c

     */
    const char *p6;
-    size_t p6_l;


Please restore. The data size is supposed to be set at all times, so we should test that it's done right.

Huh? The size is in the return_size field, the data_size field is ignored.
This doesn't get modified or used.

levitte · 2019-07-05T05:07:47Z

test/params_test.c

@@ -306,7 +301,6 @@ static BIGNUM *app_p3 = NULL;         /* "p3" */
 static unsigned char bignumbin[4096]; /* "p3" */
 static char app_p4[256];              /* "p4" */
 static char app_p5[256];              /* "p5" */
-static const char *app_p6 = NULL;     /* "p6" */


Please restore, but make it an array that initialized with app_p6_init.

I don't see how this helps, the pointer inside the OSSL_PARAM is being redirected. The buffer remains untouched.

levitte · 2019-07-05T05:08:26Z

test/params_test.c

@@ -353,8 +346,7 @@ static OSSL_PARAM static_raw_params[] = {
    { "p3", OSSL_PARAM_UNSIGNED_INTEGER, &bignumbin, sizeof(bignumbin), 0 },
    { "p4", OSSL_PARAM_UTF8_STRING, &app_p4, sizeof(app_p4), 0 },
    { "p5", OSSL_PARAM_UTF8_STRING, &app_p5, sizeof(app_p5), 0 },
-    /* sizeof(app_p6_init), because we know that's what we're using */
-    { "p6", OSSL_PARAM_UTF8_PTR, &app_p6, sizeof(app_p6_init), 0 },
+    { "p6", OSSL_PARAM_UTF8_CONST, app_p6_init, sizeof(app_p6_init), 0 },


Change app_p6_init to app_p6

levitte · 2019-07-05T05:09:36Z

test/params_test.c

-    /* sizeof(app_p6_init), because we know that's what we're using */
-    OSSL_PARAM_DEFN("p6", OSSL_PARAM_UTF8_PTR, &app_p6, sizeof(app_p6_init)),
+    OSSL_PARAM_DEFN("p6", OSSL_PARAM_UTF8_CONST, app_p6_init,
+                    sizeof(app_p6_init)),
    OSSL_PARAM_DEFN("foo", OSSL_PARAM_OCTET_STRING, &foo, sizeof(foo)),


Change app_p6_init to app_p6

levitte · 2019-07-05T05:09:50Z

test/params_test.c

-    params[n++] = OSSL_PARAM_construct_utf8_ptr("p6", (char **)&app_p6,
-                                                sizeof(app_p6_init));
+    params[n++] = OSSL_PARAM_construct_utf8_const("p6", app_p6_init,
+                                                  sizeof(app_p6_init));


Change app_p6_init to app_p6

levitte · 2019-07-05T05:10:20Z

test/params_test.c

@@ -465,7 +458,7 @@ static int test_case_variant(OSSL_PARAM *params, const struct provider_dispatch_
        || !TEST_str_eq(app_p5, p5_init)        /* "provider" value */
        || !TEST_ptr(p = OSSL_PARAM_locate(params, "p6"))
        || !TEST_size_t_eq(p->return_size, sizeof(p6_init)) /* "provider" value */
-        || !TEST_str_eq(app_p6, p6_init)        /* "provider" value */
+        || !TEST_str_eq(p->data, p6_init)        /* "provider" value */


This doesn't work. The app_p6 buffer isn't changed, the pointer that "p6" points too does. app_p6 will always have the application value.

levitte · 2019-07-05T05:10:44Z

test/params_test.c


        if (!TEST_int_eq(sneakpeek->p1, app_p1)         /* app value set */
            || !TEST_double_eq(sneakpeek->p2, p2_init)  /* Should remain untouched */
            || !TEST_BN_eq(sneakpeek->p3, app_p3)       /* app value set */
            || !TEST_str_eq(sneakpeek->p4, app_p4)      /* app value set */
            || !TEST_str_eq(sneakpeek->p5, app_p5)      /* app value set */
-            || !TEST_str_eq(sneakpeek->p6, app_p6))     /* app value set */
+            || !TEST_ptr(p = OSSL_PARAM_locate(params, "p6"))
+            || !TEST_str_eq(sneakpeek->p6, p->data))     /* app value set */


levitte · 2019-07-05T05:11:18Z

test/params_test.c

@@ -502,6 +496,9 @@ static int test_case_variant(OSSL_PARAM *params, const struct provider_dispatch_
        errcnt++;
        goto fin;
    }
+    if (!TEST_true(p = OSSL_PARAM_locate(params, "p6")))
+        goto fin;
+    sneakpeek->p6 = app_p6_init;


paulidale · 2019-07-05T06:35:58Z

I've pushed the suggested changes to the test case. The tests will fail :)

By not modifying the underlying buffer, the _CONST types behave differently to the other OSSL_PARAM types.

levitte · 2019-07-05T06:45:24Z

Hmmm, I'll have a closer look at params_test.c...

levitte · 2019-07-05T07:13:09Z

Side note: what a Whovian branch name 😉

levitte · 2019-07-05T07:28:19Z

I'm having second thoughts on this PR. While I felt positive about it for a bit, I realize that this makes the constant strings semantically radically different from anything else.

The basic idea with OSSL_PARAMs data attribute is that it points to something potentially modifiable, i.e. a responder that's supposed to pass parameters back to the caller would always modify whatever the data attribute points at. With this change, that suddenly depends on the data type, and leads to conceptual inconsistencies. This is why test/params_test.c became... weird.

I'm not quite as positive any more. Sorry.

paulidale · 2019-07-05T07:37:57Z

Slightly Whovian (although I detest that term).

https://www.youtube.com/watch?v=dSUuyUR_pPE

paulidale · 2019-07-05T07:41:14Z

The constant strings always have been different from everything else.

This bring them more in line with the other strings (only a single pointer dereference).
The difference now being between modifying the buffer and modifying the pointer to it -- the _PTR name was applicable and I'm more than happy to change back.

levitte · 2019-07-05T08:02:10Z

The constant strings always have been different from everything else.

Not in the sense that I wrote about.

paulidale · 2019-07-05T08:16:16Z

We've already established that the OSSL_PARAM can be modified.
This simplifies the handling of the PTR types.

Agreed that none of the other types store their data in the OSSL_PARAM structure but that's no reason for these two not to.

levitte · 2019-07-05T08:58:37Z

This simplifies the handling of the PTR types.

Nope.

For a caller that wants to get parameters for something, the pattern for the OSSL_PARAM array is items formed like this:

    { "key", type, &var, size, 0 },

Then, some for of get_params call, and voilà, all the variables pointed at that were relevant for the call are filled in. Just use them.

With this change, the pattern for _CONST items changes. No more pointer to a variable to be filled in, and the caller must do the extra step of either remembering the location of that particular item, or locating it with OSSL_PARAM_locate.

So all in all, I don't see it as simpler, at least for the caller.

levitte · 2019-07-05T09:00:20Z

Note that for the inconsistency I'm talking about, the mutability of OSSL_PARAM is irrelevant.

paulidale · 2019-07-05T09:24:32Z

Unfortunately, I view that pattern as a complicating negative. I doubt many people will create param array entries directly. They'll use the macros or construct calls.

Why have an extra indirection when it isn't required? To create symmetry isn't a great reason. We could always hide the & in the various macros.

levitte · 2019-07-05T10:11:27Z

The same can be said regardless of method for constructing each OSSL_PARAM, right?

    size_t foo;
    char bar[256];
    char *cookie;
    OSSL_PARAM params[4];
    size_t n = 0;

    params[n++] = OSSL_PARAM_construct_size_t("foo", &foo, sizeof(foo));
    params[n++] = OSSL_PARAM_construct_utf8_string("bar", &bar, sizeof(bar));
    params[n++] = OSSL_PARAM_construct_utf8_ptr("cookie", &cookie, sizeof(cookie));
    params[n] = OSSL_PARAM_construct_end();

    SOME_API_CTX_get_params(ctx, params);

    /* Now, simply use foo, bar and cookie */

becomes:

    size_t foo;
    char bar[256];
    char *cookie;
    OSSL_PARAM params[4], *p;
    size_t n = 0;

    params[n++] = OSSL_PARAM_construct_size_t("foo", &foo, sizeof(foo));
    params[n++] = OSSL_PARAM_construct_utf8_string("bar", &bar, sizeof(bar));
    /*
     * the address to cookie will be overwritten, we could as well do this:
     * params[n++] = OSSL_PARAM_construct_utf8_const("cookie", NULL, 0);
     */
    params[n++] = OSSL_PARAM_construct_utf8_const("cookie", &cookie, sizeof(cookie));
    params[n] = OSSL_PARAM_construct_end();

    SOME_API_CTX_get_params(ctx, params);

    p = OSSL_PARAM_locate(params, "cookie");               /* extra... */
    cookie = p->data;                                      /* treatment ... */

A variant is this:

    size_t cookie_loc;

    ...
    params[cookie_loc = n++] = OSSL_PARAM_construct_utf8_const("cookie", &cookie, sizeof(cookie));
    ...

    SOME_API_CTX_get_params(ctx, params);

    cookie = params[cookie_loc].data;

The lines after the SOME_API_CTX_get_params call is the trade-off for avoiding the extra indirection. From a user perspective, I would go "Huh? Why the extra treatment?".

Maybe I'm not seeing clearly. Do you have a code example that contradicts what I see?

richsalz · 2019-07-05T14:07:55Z

I doubt many people will create param array entries directly. They'll use the macros or construct calls.

FWIW, I have the exact opposite opinion and believe most uses will create things at compile time as much as possible. This is C. I am sad that it is not easy to create a template of param's statically and populate it with on-stack function data and then call something in a provider.

slontis · 2019-07-05T21:05:57Z

doc/man3/OSSL_PARAM.pod

@@ -274,7 +257,7 @@ could fill in the parameters like this:

    for (i = 0; params[i].key != NULL; i++) {
        if (strcmp(params[i].key, "foo") == 0) {
-            *(char **)params[i].data = "foo value";
+            params[i].data = "foo value";


Richard: are you trying to say you think that the old form
*(char **)params[i].data = "foo value";
Is not going to be confusing?

Not if you understand that the data passed is the address of the string.
But perhaps that's simply too hard to grasp, so I start to wonder if the _PTR types shouldn't simply be dropped. I don't expect them to be used very frequently anyway, so using a normal string and getting a handful of bytes copied isn't a very hard hit...

~~I'm fine dropping the _PTR types. They are only barely used at present.~~

They are used in the get_params, but the values that are being pushed into the PTR params, don't appear to be read anywhere.

paulidale · 2019-07-06T00:25:24Z

params[n++] = OSSL_PARAM_construct_utf8_string("bar", &bar, sizeof(bar));

Personally, I'd write this using a slightly different style:

params[n++] = OSSL_PARAM_construct_utf8_string("bar", bar, sizeof(bar));

The & is optional here. If the & should be mandated in this case, would it not be consistent to mandate it when referencing the address of a function? Rhetorical question that opens a different can of worms :)

If bar is dynamically allocated, the pattern breaks:

char *bar = malloc(bar_size);
params[n++] = OSSL_PARAM_construct_utf8_string("bar", bar, bar_size);

which exactly the same as the _const case.

I view the _const changes as bringing _string and _const into line -- they become extremely similar underneath -- the data field is a pointer to the data like it is for every OSSL_PARAM type. The difference being one modifies the underlying memory, the other moves the pointer. The params.h "get" functions for these types could even be merged but that's a different PR.

levitte · 2019-07-06T03:10:27Z

The difference being one modifies the underlying memory, the other moves the pointer.

Exactly. In one case, the bytes of the string is the data being passed, while in the other, the address of the string is the data being passed.

Re & or no &, that was completely beside the point I was trying to bring up.

paulidale · 2019-07-06T05:50:43Z

FWIW, I have the exact opposite opinion and believe most uses will create things at compile time as much as possible. This is C. I am sad that it is not easy to create a template of param's statically and populate it with on-stack function data and then call something in a provider.

The OpenSSL project can't preinitialise auto arrays due to compiler constraints. Static arrays referencing static variables are okay but not thread-safe. Neither of these restrictions necessarily apply to users' applications (or compilers).

My suspicion is that trying to cater for array initialisation would be a premature optimisation. There are no plans to use OSSL_PARAM arrays for anything time critical. For asymmetric crypto, the OSSL_PARAM overheads are essentially insignificant. For symmetric crypto there might be a case to answer but the critical update step won't use them. In addition, OSSL_PARAM arrays can be cached (since they will either be read or write only -- I'm not sure if this has been documented but it is the outcome of a discussion amongst the primary FIPS drones).

My honest expectation is for most application writers to copy and paste whatever is the simplest example we provide regardless of performance. This will likely result in dynamic allocation of OSSL_PARAM arrays and looking up items using the locate calls. Failing that, it will be the top result on stackoverflow :)

PS: don't be sad, life is worth far more than sadness...

richsalz · 2019-07-06T14:21:38Z

I was not clear enough. You can initialize a static array if you *don’t set the storage value* which is what I had first done. I don’t believe the path that the project has followed allows this kind of thing. You can think of it as a template-driven version if you want. :)

levitte · 2019-07-06T17:03:29Z

You can initialize a static array if you don’t set the storage value which is what I had first done.

So that is to say, create an automatic (not static) array and initialize it as much as possible, just to fill in the few values you can in code? Something like this:

     OSSL_PARAM params[] = {
        { "foo", OPENSSL_PARAM_INTEGER, NULL, sizeof(foo), 0 },
        { "bar", OPENSSL_PARAM_UFT8_STRING, NULL, sizeof(bar), 0 },
        { "cookie", OPENSSL_PARAM_OCTET_PTR, NULL, 0, 0 },
        { NULL, 0, NULL, 0, 0 },
    };

    params[0].data = &foo; 
    params[1].data = bar; /* bar is an array */ 
    params[2].data = cookie; /* cookie is a pointer */ 
    params[2].data_size = cookie_n;

You can still do this. There are helper macros that help as well.

I don’t believe the path that the project has followed allows this kind of thing.

The above is still permitted. Having the value that are more dynamic in nature in the initializer has turned out not to be permitted (or at least well defined) in ISO C90.

richsalz · 2019-07-06T18:12:52Z

You can still do this. There are helper macros that help as well.

You can come close, it's not the same, but oh well.

levitte · 2019-07-06T18:56:36Z

I'm at a loss to understand what you're asking for, then.

richsalz · 2019-07-06T18:58:25Z

Never mind, just treat it as a bitter old man grumbling that his code was rejected.

levitte · 2019-07-06T18:59:30Z

doc/man3/OSSL_PARAM.pod

 Note that the use of this type is B<fragile> and can only be safely
 used for data that remains constant and in a constant location for a
 long enough duration (such as the life-time of the entity that
 offers these parameters).

-=item C<OSSL_PARAM_OCTET_PTR>
+=item C<OSSL_PARAM_OCTET_CONST>

 The parameter data is a pointer to an arbitrary string of bytes.


This sentence should be changed the same way it was for OSSL_PARAM_UTF8_CONST

levitte · 2019-07-06T18:59:51Z

doc/man3/OSSL_PARAM.pod


 Note that the use of this type is B<fragile> and can only be safely
 used for data that remains constant and in a constant location for a
 long enough duration (such as the life-time of the entity that
 offers these parameters).

+


levitte · 2019-07-08T04:03:31Z

After some conversation with @paulidale, I figured I was a bit too stuck in my ideas, and that if a trade-off is for the OSSL_PARAM owner to have a location variable instead of a pointer variable to achieve the same thing, I can live with it.

paulidale · 2019-07-08T21:13:41Z

After further discussion, I'm closing this PR as something that seemed like a reasonable idea but didn't quite work as expected.

paulidale added 2 commits July 4, 2019 21:14

Extra level of indirection for string _PTR param types removed.

f8dd850

The two _PTR string types have had the indirection removed. The pointer to their data lives inside the OSSL_PARAM structure without needing an intermediate pointer.

doc-nits fixes

68a66e9

levitte requested changes Jul 5, 2019

View reviewed changes

make review suggested changes to the test case -- NB: this fails

3a80813

update documentation

78b825d

slontis reviewed Jul 5, 2019

View reviewed changes

levitte requested changes Jul 6, 2019

View reviewed changes

paulidale closed this Jul 8, 2019

paulidale deleted the exterminate-ptr branch August 15, 2019 01:51

Extra level of indirection for string _PTR param types removed. #9313

Extra level of indirection for string _PTR param types removed. #9313

Conversation

paulidale commented Jul 5, 2019

Choose a reason for hiding this comment

paulidale Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Jul 5, 2019 • edited Loading

levitte commented Jul 5, 2019

levitte commented Jul 5, 2019

levitte commented Jul 5, 2019

paulidale commented Jul 5, 2019

paulidale commented Jul 5, 2019

levitte commented Jul 5, 2019

paulidale commented Jul 5, 2019

levitte commented Jul 5, 2019

levitte commented Jul 5, 2019

paulidale commented Jul 5, 2019

levitte commented Jul 5, 2019

richsalz commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Jul 6, 2019

levitte commented Jul 6, 2019

paulidale commented Jul 6, 2019

richsalz commented Jul 6, 2019 via email

levitte commented Jul 6, 2019

richsalz commented Jul 6, 2019

levitte commented Jul 6, 2019

richsalz commented Jul 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Jul 8, 2019 • edited Loading

paulidale commented Jul 8, 2019

paulidale Jul 5, 2019 •

edited

Loading

paulidale commented Jul 5, 2019 •

edited

Loading

paulidale Jul 8, 2019 •

edited

Loading

levitte commented Jul 8, 2019 •

edited

Loading