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);
 }

Sashan

few finishing touches are still needed. thanks.

Sashan · 2025-11-06T22:23:33Z

source/x509storeissuer.c

+                    path);
+        }
+
+        if (pool_cnt != NULL);


I think the ; should not be here.

Thanks, I wonder how it hasn't been caught, as it indeed should be called with pool_cnt == NULL if mode == MODE_R.

Sashan · 2025-11-06T22:29:27Z

source/x509storeissuer.c

+    timeout_s = strtod(optarg, &endptr);
+
+    if (endptr == NULL || *endptr != '\0' || timeout_s < 0)
+        errx(EXIT_FAILURE, "incorrect timeout value: \"%s\"");


there is %s in format string. shall we add ``...", timeout_s); here?

Yes, I wonder why hasn't it been caught by the compiler, __attribute__((format(printf))) and/or -Wmissing-format-attribute are missing.

Sashan · 2025-11-06T22:30:35Z

source/x509storeissuer.c

+            printf(": avg: %9.3lf us, median: %9.3lf us"
+                   ", min: %9.3lf us @thread %3zu, max: %9.3lf us @thread %3zu"
+                   ", stddev: %9.3lf us (%8.4lf%%)"
+                   ", hits %9zu of %9zu (%8.4lf%%)"


the types are uint64_t the format string should %llu

681 ", hits %9llu of %9llu (%8.4lf%%)" 682 ", added certs: %llu\n",

Good catch, the proper format specifier for uint64_t is "%s" PRIu64, however.

Sashan · 2025-11-06T22:34:11Z

source/evp_fetch.c

 OSSL_TIME max_time;

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


If I rememer correctly the change was prompted by thread affinity which is gone. so perhaps we can drop this changeset from this PR too.

esyr · 2025-11-07T14:37:41Z

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

Interesting, does OpenBSD use vdso for gettimeofday() calls? Otherwise, while being apparently useless for MacOS, it might make sense to reduce the rate of ossl_time_now() calls, after all. May I ask you to try to use different values other than 0x3f in that old version ([1], I guess?) to see if it has an impact for you?

[1] https://github.com/openssl/perftools/commits/ccbb791

Sashan · 2025-11-07T17:41:57Z

Interesting, does OpenBSD use vdso for gettimeofday() calls? Otherwise, while being apparently useless for MacOS, it

If I understand vdso description right then OpenBSD uses very similar way to implement gettime*() functions. The magic happens in libc on OpenBSD details can be found in this email thread

might make sense to reduce the rate of ossl_time_now() calls, after all. May I ask you to try to use different values other than 0x3f in that old version ([1], I guess?) to see if it has an impact for you?

[1] https://github.com/openssl/perftools/commits/ccbb791

this what I get for the main branch s found on github:

lifty$ ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8  
25.113781
lifty$ time ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8 
24.919960
    0m05.05s real     0m04.65s user     0m34.88s system

This is what I get for the old branch with 0x3f value

lifty$ time ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8 
8.745155
    0m06.17s real     0m38.15s user     0m01.29s system

this what I get for 0x3ff

lifty$ time ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8 
8.749818
    0m06.05s real     0m37.40s user     0m01.51s system

this result is with 0x3ffff

lifty$ time ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8 
8.428343
    0m07.04s real     0m42.01s user     0m00.92s system

this is with value 0

lifty$ time ./x509storeissuer -t /home/sashan/work.openssl/QUIC/openssl.addr_validation/test/certs/ 8 
8.952559
    0m06.17s real     0m38.21s user     0m01.05s system

esyr · 2025-11-07T18:20:58Z

Well, looking at these, I'd say it's almost definitely the case that ossl_time_now() calls eat significant part of the run time (over 70%, judging by the ratio between user and system time and changes in the measured time) and taint the measurements.