x509storeissuer update #50

esyr · 2025-09-23T14:45:57Z

This patch set slightly updates the x509storeissuer test to actually populate the store with certificates, and provides additional modes of operation and measurement data (when requested).

Resolves: openssl/project#1529

Sashan · 2025-10-14T06:15:17Z

source/x509storeissuer.c

+    td->q_data[quantiles - 1].count = count;
+    td->q_data[quantiles - 1].end_time = time;
+
 err:


ci seems to be unhappy because of this. I would try to add return; or just empty statement ;. finge cross.

Sashan

May be I would move the recent changeset which populates store with certificates to separate PR. so we can get at least some changes in and fine tune new set of changes. that's just suggestions. but it feels like here we are just piling up more and more code.

Sashan · 2025-10-23T07:50:33Z

source/x509storeissuer.c

 */

-#include <stdlib.h>
+#include <dirent.h>


I'm afraid including dirent.h won't fly on windows.

Fair enough, made it fly.

nhorman · 2025-10-24T12:21:31Z

source/x509storeissuer.c

+    static unsigned char out[64];
+    size_t len;
+
+    len = rand() % (sizeof(out) - 2) + 1;


What happens here if rand() returns a value that is a multiple of sizeof(out) - 2? Don't we then just get an empty string for the issuer or subject name? Is that a problem?

That's why +1 at the end.

nhorman · 2025-10-24T12:28:48Z

source/x509storeissuer.c

+        fprintf(stderr, "Added %zu generated certificates to the store\n",
+                num_store_gen_certs);
+
+    num_certs += read_certsdirs(argv + dirs_start, argc - dirs_start - 1,


So, I'm a bit confused here. Most of this code that targets x509storeissuer is about dynamically generating certificates at run time. Are we also allowing the loading of a certificate directory from disk? If we are allowing both, would it make more sense to move the dynamic generation to an external script in the repository, so that we can just support reading from disk in the code, and create any dynamic certs via the aforementioned script?

I guess it makes sense. The idea behind generating them in the test was to have control over the contents of the certificates, but that can just as well be achieved by an external script.

The loading has been reworked to include -l/-L/-E options, the certificate generation patches have been dropped.

npajkovsky · 2025-10-24T12:40:44Z

source/perflib/err.h


-#include <stdarg.h>
+# if !defined(_WIN32)
+


nit: we don't do new lines around the ifdefs

npajkovsky · 2025-10-24T12:42:59Z

source/x509storeissuer.c

-        goto err;
-    }
+    if (cert == NULL)
+        errx(EXIT_FAILURE, "Failed to allocate cert path");


You're missing goto err on every errx.

errx() does exit() so goto won't be executed.

npajkovsky · 2025-10-24T12:44:54Z

source/x509storeissuer.c

+
+    return ret;
+}
+


Can you move it to libperf? It's used in other tests, and that should be fixed in other tests in another PR.

Will do a separate PR for that.

npajkovsky · 2025-10-24T13:29:15Z

source/x509storeissuer.c

    fprintf(stderr,
            "Usage: %s [-t] [-v] [-T time] [-n nonce_type:type_args] "
-            "certsdir threadcount\n"
+            "certsdir [certsdir...] threadcount\n"


Why do we need another certsdir?

Initially it was in order to avoid the need to combine the cert dirs outside the test, now it's more to get ability to point to a generated certs dir in addition to initially loaded ones.

npajkovsky · 2025-10-24T13:31:08Z

source/x509storeissuer.c

        counts[num]++;
-        time = ossl_time_now();
+        if ((count & 0x3f) == 0)
+            time = ossl_time_now();


Please keep it aligned with other tests. If there is an issue, create a new ticket where we can discuss it.

Since the were no observed benefit on MacOS after all, the patch has been dropped.

Sashan

looks good in general. I could spot few nits which I think are worth to address.

Sashan · 2025-10-24T08:24:00Z

source/x509storeissuer.c

-    if (ctx == NULL || !X509_STORE_CTX_init(ctx, store, x509, NULL)) {
-        printf("Failed to initialise X509_STORE_CTX\n");
-        err = 1;
+    alg = p ? *alg_storage = OPENSSL_strndup(algstr, p - algstr) : algstr;


I wonder if there should also be OPENSSL_strdup(algstr) in false branch.

The offending code has been dropped after the latest changes.

Sashan · 2025-10-24T08:26:29Z

source/x509storeissuer.c

+    return ctx;
+
+err:
+    EVP_PKEY_CTX_free(ctx);


shall we do

OPENSSL_free(alg); *alg_storage = NULL;

it does not matter much now because program is going to terminate when failure happens here.

Ditto here.

Sashan · 2025-10-24T13:57:36Z

source/x509storeissuer.c

+    static unsigned char out[64];
+    size_t len;
+
+    len = rand() % (sizeof(out) - 2) + 1;


shall we care if rand() returns 0 here making the function to yield an empty string?

Ditto here.

Sashan · 2025-10-24T14:01:26Z

source/x509storeissuer.c

+    int error = 1;
+
+    cert = X509_new();
+    if (!cert) {


NIT: in function make_pkey_ctx() we use == NULL / != NULL to compare pointers. either way is fine with me as long as it is used consistently. Well I have slight preference to use == NULL over !x
thanks.

Ditto here.

Sashan · 2025-10-24T14:05:56Z

source/x509storeissuer.c

+    X509 *x509_nonce = X509_new();
+    X509_NAME *x509_name_nonce = NULL;
+
+    if (!x509_nonce)


== NULL ?

Ditto here.

Sashan · 2025-10-24T14:07:14Z

source/x509storeissuer.c

+#if defined(_WIN32)
+    /*
+     * So, we don't try to concatenate the provided path with the directory
+     * paths if the path start with the following:


I think it should be ...the path starts

Sashan · 2025-10-24T14:17:00Z

source/x509storeissuer.c

+{
+    X509 *x509 = NULL;
+    char *path = NULL;
+    size_t ret = 1;


I think ret should be initialized to 0 here.

Yes, that's correct, fixed.

Sashan · 2025-10-24T14:38:01Z

source/x509storeissuer.c

+static void
+do_x509storeissuer(size_t num)
+{
+    struct thread_data *td = thread_data + num;


would it be possible to do td = &thread_data[num]; thanks.

npajkovsky · 2025-10-27T10:25:58Z

openssl head 0ab2ece0a2025ebdea1f94744424ef89ffb9a2b0

./Configure --banner=Configured --strict-warnings

Main branch

$ ./x509storeissuer -t /Users/npajkovsky/openssl/openssl/test/certs 8
1.981555

Your branch

$ ./x509storeissuer -t /Users/npajkovsky/openssl/openssl/test/certs 8
4.663879

There is something that changes the behaviour of the test that we already have publicly available.

Sashan · 2025-10-27T11:22:08Z

hmm, this is actually a very good point. I think the whole statistics machinery deserves some explanation in comment.

I see the difference, but otherway round:

lifty$ ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.sashan/test/certs/ 8
10.218173
lifty$ cd ../build-main/
lifty$ ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.sashan/test/certs/ 8
25.938117

Sashan · 2025-10-27T11:29:02Z

source/x509storeissuer.c

+            issuer = NULL;
+        }
+
+        count++;


I found the code block which starts at 769 as pretty dense. it perhaps deserves some comment here. my understanding is that the whole idea is to divide the collection of statistics to evenly distributed samples (quantiles). The current setting is there are 5 quantiles for the whole duration of test which is 5 secs.

the line here just makes we check the elapsed time only once in a while (count 0x3f). If time collection interval ellapses, then we save a sample (quantile) for elapsed interval.

and one important detail to mention here is that when tool is running with -t option, terse mode there is just one quantile (quantile = 1) which covers the whole test duration. no split to evenly distributed intervals happens.

Sashan · 2025-10-27T11:35:15Z

source/x509storeissuer.c

+                td->q_data[q].found = found;
+                td->q_data[q].added_certs = add;
+                td->q_data[q].end_time = time;
+                q_end.t = (duration.t * (++q + 1)) / quantiles + td->start_time.t;


here we calculate the time when to collect the next sample. ++q + 1 prevents 0 (which might be interpreted as now in current logic at line 771.

nhorman · 2025-10-27T11:49:17Z

@npajkovsky given that this change introduces a population of the X509_STORE that we are testing (i.e. with this change we're actually iterating over lots of certificates. vs no certificates), I would expect some level of run time increase (though a 4x slowdown is odd). Weather or not we should set the default NUM_KEYS value to 16 vs 0 is probably the question to answer here.

esyr · 2025-10-27T12:03:39Z

@npajkovsky given that this change introduces a population of the X509_STORE that we are testing (i.e. with this change we're actually iterating over lots of certificates. vs no certificates), I would expect some level of run time increase (though a 4x slowdown is odd). Weather or not we should set the default NUM_KEYS value to 16 vs 0 is probably the question to answer here.

I'm going to change the defaults so it still measures the time against an empty store, so the additional measurements can be added separately.

npajkovsky · 2025-10-27T13:57:53Z

@npajkovsky given that this change introduces a population of the X509_STORE that we are testing (i.e. with this change we're actually iterating over lots of certificates. vs no certificates), I would expect some level of run time increase (though a 4x slowdown is odd). Weather or not we should set the default NUM_KEYS value to 16 vs 0 is probably the question to answer here.

Number of certificates in the store should not matter much. X509_STORE is using hastable which maps certificate's subject to STACK_OF(X509_OBJECTS). Hence, the only way how to iterate over certificates is to have plenty of certificates with the same subject. I don't think we have this case right now.

npajkovsky · 2025-10-27T16:51:04Z

The difference between performance is because the original code didn't find the certificate, while @esyr code finds it. That triggers a bit of different code paths which we can measure.

do_x509storeissuer should really be named do_x509storeissuer_cache_miss. Your implementation should be a cache hit.

npajkovsky · 2025-10-29T10:38:15Z

Also, there are two leaks.

diff --git a/source/x509storeissuer.c b/source/x509storeissuer.c
index 3acce441e2a2..6c737bf578c7 100644
--- a/source/x509storeissuer.c
+++ b/source/x509storeissuer.c
@@ -714,6 +714,8 @@ read_certsdir(char * const dir, X509_STORE * const store)
         cnt += read_cert(dir, e->d_name, store);
     }

+    closedir(d);
+
     return cnt;
 }
 #endif /* defined(_WIN32) */
@@ -941,6 +943,8 @@ report_result(int verbosity)
         }
         break;
     }
+
+    OPENSSL_free(times);
 }

Signed-off-by: Eugene Syromiatnikov <[email protected]>

… function Signed-off-by: Eugene Syromiatnikov <[email protected]>

Signed-off-by: Eugene Syromiatnikov <[email protected]>

…ration Signed-off-by: Eugene Syromiatnikov <[email protected]>

Signed-off-by: Eugene Syromiatnikov <[email protected]>

…mmering the common counts array's cache line Signed-off-by: Eugene Syromiatnikov <[email protected]>

…nt and report successful calls Signed-off-by: Eugene Syromiatnikov <[email protected]>

…e store Signed-off-by: Eugene Syromiatnikov <[email protected]>

Signed-off-by: Eugene Syromiatnikov <[email protected]>

…bosity level is DEBUG_STATS or higher Signed-off-by: Eugene Syromiatnikov <[email protected]>

Signed-off-by: Eugene Syromiatnikov <[email protected]>

…the run Signed-off-by: Eugene Syromiatnikov <[email protected]>

…t run matrix Signed-off-by: Eugene Syromiatnikov <[email protected]>

…e in CI Signed-off-by: Eugene Syromiatnikov <[email protected]>

bob-beck · 2025-11-13T17:40:20Z

source/x509storeissuer.c

+
+    do {
+        if (td->cert_count > 0 && (rand() % 65536 < w_probability)) {
+            size_t cert_id = add % td->cert_count;


What was the license on the code you were borrowing from FreeBSD?

The normal license on FreeBSD is copyright the freebsd foundation and is 2 clause BSD. You can't
just copy code from FreeBSD that is licensed like that, and put it in something that is Apache2 licensed copyright the OpenSSL authors. You would have to ask permission to relicense like that unless we
include the original license with the code and license such code like that.

Do we really need this level of complexity for this test?

bob-beck · 2025-11-13T17:44:23Z

source/perflib/err.h

+extern void vwarn(const char *, va_list);
+
+extern ossl_noreturn void errx(int, const char *, ...);
+extern ossl_noreturn void err(int, const char *, ...);


Adding this dependency here seems awfully intrusive in the rest of the code, for what you're
actually getting out of it for this that couldn't be done in other ways.

If we decide we want this (and I'm a BSD guy so not overly objectiing to them) I think this should be
a decision to deal with this separately from improvements to this test.

bob-beck · 2025-11-13T17:45:27Z

source/evp_fetch.c

 OSSL_TIME max_time;

-int err = 0;
+int error = 0;


As mentioned, this is pretty intrusive and should probably be a separate change from this.

like with a real issue and stuff about the name of this global symbol in this file and what it conflicts with. To me it seems like changing err to error is just moving the deck chair over one place and waiting for the next conflict. While making a decision to solve that is fine, we should solve it rather than just hacking this just for the purpose of having err() in this test.

bob-beck · 2025-11-13T17:49:12Z

source/perflib/noreturn.h

+ */
+
+#ifndef OSSL_PERFLIB_NORETURN_H
+# define OSSL_PERFLIB_BASENAME_H


This appears to not be guarded, as. you're defining something different than the guard.

bob-beck · 2025-11-13T17:51:50Z

source/x509storeissuer.c

+#else
+# define ALIGN64
+#endif
+


Got a measured perfomance difference with and without?

bob-beck · 2025-11-13T17:57:16Z

source/x509storeissuer.c

-    if (!perflib_run_multi_thread_test(do_x509storeissuer, threadcount, &duration)) {
-        printf("Failed to run the test\n");
-        goto err;
+    x509_nonce = make_nonce(&nonce_cfg);


So this can return NULL, and is already checked.

so that function does not need noreturn, and therefore I don't think you also need to bring in the noreturn stuff just for this.

bob-beck · 2025-11-13T18:08:26Z

source/x509storeissuer.c


    ret = EXIT_SUCCESS;

- err:


I think changing this sort of stuff to errx is probably a regression if the tests are used to check leaks, it just becomes a pain. So I would avoid adding more of this type of refactor until we know what our answer is for that, and just keep these as goto err;

Additionaly because a lot of what appears to be being brought in here for err/errx is a lot of unrelated churn for this. and makes this change a lot more intrusive than we need otherwise.

If we truly want err/errx in tests we should make an issue for it, and add it separately, with some documentation on it at least in it's header file for internal use here, and a PR to add it, before using it. Adding it while doing this seems to add a lot of baggage and complexity to this PR that I don't think is needed.

FYI: err/errx has been added some time ago:
#26

the question rather is to drop the changeset which does s/err/error exercise. this changeset came from here as an attempt to fix some linker/compiler warning on windows. The compile/warning was triggered by code which no longer exists here. so there is some history in this PR...

Sashan requested changes Oct 14, 2025

View reviewed changes

esyr force-pushed the esyr/x509storeissuer-update branch 2 times, most recently from d1454e0 to b327824 Compare October 15, 2025 14:30

npajkovsky mentioned this pull request Oct 21, 2025

Add evp_hash perftool #58

Closed

esyr force-pushed the esyr/x509storeissuer-update branch from b327824 to 1206f0b Compare October 22, 2025 14:55

esyr marked this pull request as ready for review October 22, 2025 14:56

esyr force-pushed the esyr/x509storeissuer-update branch from 1206f0b to 3001309 Compare October 22, 2025 15:01

esyr requested review from Sashan and npajkovsky October 22, 2025 15:01

Sashan requested changes Oct 23, 2025

View reviewed changes

esyr force-pushed the esyr/x509storeissuer-update branch 3 times, most recently from ccbb791 to 4b335f7 Compare October 23, 2025 15:33

esyr requested a review from Sashan October 24, 2025 01:35

npajkovsky requested a review from nhorman October 24, 2025 07:05

nhorman reviewed Oct 24, 2025

View reviewed changes

npajkovsky reviewed Oct 24, 2025

View reviewed changes

Sashan requested changes Oct 24, 2025

View reviewed changes

Sashan reviewed Oct 27, 2025

View reviewed changes

esyr force-pushed the esyr/x509storeissuer-update branch 2 times, most recently from 0b96651 to 44e0c66 Compare November 5, 2025 15:12

esyr requested a review from Sashan November 5, 2025 15:12

esyr requested a review from nhorman November 11, 2025 11:05

esyr and others added 24 commits November 11, 2025 12:30

x509storeissuer: factor out usage printing

6398216

Signed-off-by: Eugene Syromiatnikov <[email protected]>

s/err/error where apropriate easiest way to fix liner issues on windows

15c799b

Use perflib/err.h unconditionally

7448789

Signed-off-by: Eugene Syromiatnikov <[email protected]>

perflib: add vwarn/err/warn

ee5fd86

Signed-off-by: Eugene Syromiatnikov <[email protected]>

perflib: add ossl_noreturn qualifier and use it for err/errx

6b108da

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: use errx/warnx for error message output

2b7fcf4

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: factor out integer parameter parsing into a separate…

68a7b91

… function Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add ability to configure verbosity level

9c03311

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: provide the initial infrastructure for nonce configu…

be5de07

…ration Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: unify funtion definition formatting

2cd00eb

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add -T option to specify the run time

18fab94

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: support providing multiple certificate directories

b9e8a78

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: count the iterations in a local variable to avoid ha…

09f24e3

…mmering the common counts array's cache line Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: tolerate X509_STORE_CTX_get1_issuer() successes, cou…

92ca087

…nt and report successful calls Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add certificates from the provided directories to th…

8a68e53

…e store Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add and option to configure X509_STORE_CTX sharing

319e6ec

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: report the store size before the test run if the ver…

aec1748

…bosity level is DEBUG_STATS or higher Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: make the verbose reporting more elaborate

ca94c96

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: make thread_data cache-line-aligned

a600b7e

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add limits for certificate file/directory load counts

5acec49

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add an ability to skip loading default paths

753613f

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: store excess certificates in a pool for later use

5444f48

Signed-off-by: Eugene Syromiatnikov <[email protected]>

x509storeissuer: add ability to add certificates to the store during …

8ba8e6b

…the run Signed-off-by: Eugene Syromiatnikov <[email protected]>

CMakeLists.txt: add recently added x509storeissuer options to the tes…

176619a

…t run matrix Signed-off-by: Eugene Syromiatnikov <[email protected]>

esyr force-pushed the esyr/x509storeissuer-update branch from 45e4a5f to 176619a Compare November 11, 2025 16:11

CMakeLists.txt, test.yml: make "run" target optional, continute to us…

14fa76e

…e in CI Signed-off-by: Eugene Syromiatnikov <[email protected]>

bob-beck suggested changes Nov 13, 2025

View reviewed changes

esyr closed this Nov 18, 2025

x509storeissuer update #50

x509storeissuer update #50

Uh oh!

Conversation

esyr commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sashan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esyr Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sashan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npajkovsky commented Oct 27, 2025

Uh oh!

Sashan commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

esyr commented Sep 23, 2025 •

edited

Loading

esyr Oct 24, 2025 •

edited

Loading

Sashan commented Oct 27, 2025 •

edited

Loading

Sashan Oct 27, 2025 •

edited

Loading

npajkovsky commented Oct 27, 2025 •

edited

Loading

npajkovsky commented Oct 29, 2025 •

edited

Loading