From f0c9b3aeacacf11d55ea928d1ed98c98b4b9d1e9 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Thu, 14 Nov 2024 10:44:42 -0800 Subject: [PATCH] Revert "Replace CONF's internal representation with something more typesafe" (#1986) This reverts commit cc932cf394d2af845fdc46eac14f21886d5c98c9. OpenSSL's historically made the `CONF` struct non-opaque. This hasn't changed in OpenSSL 3 either although there is documentation that indicates they wish to do so in the long run. It's likely going to be some time before this is actually done, but in the meantime Ruby has a dependency on `CONF`'s internals. This commit is reverting some of the original upstream work done that changes the internal variables of `CONF`. There will be a follow up PR to make the structure non-opaque. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- crypto/conf/conf.c | 174 ++++++++++++++++++++------------------- crypto/conf/conf_test.cc | 3 +- crypto/conf/internal.h | 6 +- 3 files changed, 90 insertions(+), 93 deletions(-) diff --git a/crypto/conf/conf.c b/crypto/conf/conf.c index 7a7a3808e5..7e049bb303 100644 --- a/crypto/conf/conf.c +++ b/crypto/conf/conf.c @@ -70,51 +70,48 @@ #include "../internal.h" -struct conf_section_st { - char *name; - // values contains non-owning pointers to the values in the section. - STACK_OF(CONF_VALUE) *values; -}; - static const char kDefaultSectionName[] = "default"; -static uint32_t conf_section_hash(const CONF_SECTION *s) { - return OPENSSL_strhash(s->name); -} - -static int conf_section_cmp(const CONF_SECTION *a, const CONF_SECTION *b) { - return strcmp(a->name, b->name); -} - static uint32_t conf_value_hash(const CONF_VALUE *v) { - const uint32_t section_hash = OPENSSL_strhash(v->section); - const uint32_t name_hash = OPENSSL_strhash(v->name); + const uint32_t section_hash = v->section ? OPENSSL_strhash(v->section) : 0; + const uint32_t name_hash = v->name ? OPENSSL_strhash(v->name) : 0; return (section_hash << 2) ^ name_hash; } static int conf_value_cmp(const CONF_VALUE *a, const CONF_VALUE *b) { - int cmp = strcmp(a->section, b->section); - if (cmp != 0) { - return cmp; + int i; + + if (a->section != b->section) { + i = strcmp(a->section, b->section); + if (i) { + return i; + } } - return strcmp(a->name, b->name); + if (a->name != NULL && b->name != NULL) { + return strcmp(a->name, b->name); + } else if (a->name == b->name) { + return 0; + } else { + return (a->name == NULL) ? -1 : 1; + } } CONF *NCONF_new(void *method) { + CONF *conf; + if (method != NULL) { return NULL; } - CONF *conf = OPENSSL_malloc(sizeof(CONF)); + conf = OPENSSL_malloc(sizeof(CONF)); if (conf == NULL) { return NULL; } - conf->sections = lh_CONF_SECTION_new(conf_section_hash, conf_section_cmp); - conf->values = lh_CONF_VALUE_new(conf_value_hash, conf_value_cmp); - if (conf->sections == NULL || conf->values == NULL) { - NCONF_free(conf); + conf->data = lh_CONF_VALUE_new(conf_value_hash, conf_value_cmp); + if (conf->data == NULL) { + OPENSSL_free(conf); return NULL; } @@ -123,64 +120,69 @@ CONF *NCONF_new(void *method) { CONF_VALUE *CONF_VALUE_new(void) { return OPENSSL_zalloc(sizeof(CONF_VALUE)); } -static void value_free(CONF_VALUE *value) { - if (value == NULL) { - return; - } +static void value_free_contents(CONF_VALUE *value) { OPENSSL_free(value->section); - OPENSSL_free(value->name); - OPENSSL_free(value->value); - OPENSSL_free(value); + if (value->name) { + OPENSSL_free(value->name); + OPENSSL_free(value->value); + } else { + // TODO(davidben): When |value->name| is NULL, |CONF_VALUE| is actually an + // entirely different structure. This is fragile and confusing. Make a + // proper |CONF_SECTION| type that doesn't require this. + sk_CONF_VALUE_free((STACK_OF(CONF_VALUE) *)value->value); + } } -static void section_free(CONF_SECTION *section) { - if (section == NULL) { - return; +static void value_free(CONF_VALUE *value) { + if (value != NULL) { + value_free_contents(value); + OPENSSL_free(value); } - OPENSSL_free(section->name); - sk_CONF_VALUE_free(section->values); - OPENSSL_free(section); } static void value_free_arg(CONF_VALUE *value, void *arg) { value_free(value); } -static void section_free_arg(CONF_SECTION *section, void *arg) { - section_free(section); -} - void NCONF_free(CONF *conf) { - if (conf == NULL) { + if (conf == NULL || conf->data == NULL) { return; } - lh_CONF_SECTION_doall_arg(conf->sections, section_free_arg, NULL); - lh_CONF_SECTION_free(conf->sections); - lh_CONF_VALUE_doall_arg(conf->values, value_free_arg, NULL); - lh_CONF_VALUE_free(conf->values); + lh_CONF_VALUE_doall_arg(conf->data, value_free_arg, NULL); + lh_CONF_VALUE_free(conf->data); OPENSSL_free(conf); } -static CONF_SECTION *NCONF_new_section(const CONF *conf, const char *section) { - CONF_SECTION *s = OPENSSL_malloc(sizeof(CONF_SECTION)); - if (!s) { - return NULL; +static CONF_VALUE *NCONF_new_section(const CONF *conf, const char *section) { + STACK_OF(CONF_VALUE) *sk = NULL; + int ok = 0; + CONF_VALUE *v = NULL, *old_value; + + sk = sk_CONF_VALUE_new_null(); + v = CONF_VALUE_new(); + if (sk == NULL || v == NULL) { + goto err; } - s->name = OPENSSL_strdup(section); - s->values = sk_CONF_VALUE_new_null(); - if (s->name == NULL || s->values == NULL) { + v->section = OPENSSL_strdup(section); + if (v->section == NULL) { goto err; } - CONF_SECTION *old_section; - if (!lh_CONF_SECTION_insert(conf->sections, &old_section, s)) { + v->name = NULL; + v->value = (char *)sk; + + if (!lh_CONF_VALUE_insert(conf->data, &old_value, v)) { goto err; } - section_free(old_section); - return s; + value_free(old_value); + ok = 1; err: - section_free(s); - return NULL; + if (!ok) { + sk_CONF_VALUE_free(sk); + OPENSSL_free(v); + v = NULL; + } + return v; } static int str_copy(CONF *conf, char *section, char **pto, char *from) { @@ -252,20 +254,21 @@ static int str_copy(CONF *conf, char *section, char **pto, char *from) { return 0; } -static CONF_SECTION *get_section(const CONF *conf, const char *section) { - CONF_SECTION template; +static CONF_VALUE *get_section(const CONF *conf, const char *section) { + CONF_VALUE template; + OPENSSL_memset(&template, 0, sizeof(template)); - template.name = (char *) section; - return lh_CONF_SECTION_retrieve(conf->sections, &template); + template.section = (char *) section; + return lh_CONF_VALUE_retrieve(conf->data, &template); } const STACK_OF(CONF_VALUE) *NCONF_get_section(const CONF *conf, const char *section) { - const CONF_SECTION *section_obj = get_section(conf, section); - if (section_obj == NULL) { + const CONF_VALUE *section_value = get_section(conf, section); + if (section_value == NULL) { return NULL; } - return section_obj->values; + return (STACK_OF(CONF_VALUE)*) section_value->value; } const char *NCONF_get_string(const CONF *conf, const char *section, @@ -277,35 +280,30 @@ const char *NCONF_get_string(const CONF *conf, const char *section, } OPENSSL_memset(&template, 0, sizeof(template)); - template.section = (char *)section; - template.name = (char *)name; - value = lh_CONF_VALUE_retrieve(conf->values, &template); + template.section = (char *) section; + template.name = (char *) name; + value = lh_CONF_VALUE_retrieve(conf->data, &template); if (value == NULL) { return NULL; } return value->value; } -static int add_string(const CONF *conf, CONF_SECTION *section, +static int add_string(const CONF *conf, CONF_VALUE *section, CONF_VALUE *value) { - value->section = OPENSSL_strdup(section->name); - if (value->section == NULL) { - return 0; - } + STACK_OF(CONF_VALUE) *section_stack = (STACK_OF(CONF_VALUE)*) section->value; + CONF_VALUE *old_value; - if (!sk_CONF_VALUE_push(section->values, value)) { + value->section = OPENSSL_strdup(section->section); + if (!sk_CONF_VALUE_push(section_stack, value)) { return 0; } - CONF_VALUE *old_value; - if (!lh_CONF_VALUE_insert(conf->values, &old_value, value)) { - // Remove |value| from |section->values|, so we do not leave a dangling - // pointer. - sk_CONF_VALUE_pop(section->values); + if (!lh_CONF_VALUE_insert(conf->data, &old_value, value)) { return 0; } if (old_value != NULL) { - (void)sk_CONF_VALUE_delete_ptr(section->values, old_value); + (void)sk_CONF_VALUE_delete_ptr(section_stack, old_value); value_free(old_value); } @@ -390,8 +388,8 @@ int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { int again; long eline = 0; char btmp[DECIMAL_SIZE(eline) + 1]; - CONF_VALUE *v = NULL; - CONF_SECTION *sv = NULL; + CONF_VALUE *v = NULL, *tv; + CONF_VALUE *sv = NULL; char *section = NULL, *buf; char *start, *psection, *pname; @@ -542,7 +540,6 @@ int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { goto err; } - CONF_SECTION *tv; if (strcmp(psection, section) != 0) { if ((tv = get_section(conf, psection)) == NULL) { tv = NCONF_new_section(conf, psection); @@ -572,7 +569,12 @@ int NCONF_load_bio(CONF *conf, BIO *in, long *out_error_line) { } snprintf(btmp, sizeof btmp, "%ld", eline); ERR_add_error_data(2, "line ", btmp); - value_free(v); + + if (v != NULL) { + OPENSSL_free(v->name); + OPENSSL_free(v->value); + OPENSSL_free(v); + } return 0; } diff --git a/crypto/conf/conf_test.cc b/crypto/conf/conf_test.cc index d965f25b7f..92e52db5f9 100644 --- a/crypto/conf/conf_test.cc +++ b/crypto/conf/conf_test.cc @@ -95,8 +95,7 @@ static void ExpectConfEquals(const CONF *conf, const ConfModel &model) { // There should not be any other values in |conf|. |conf| currently stores // both sections and values in the same map. - EXPECT_EQ(lh_CONF_SECTION_num_items(conf->sections), model.size()); - EXPECT_EQ(lh_CONF_VALUE_num_items(conf->values), total_values); + EXPECT_EQ(lh_CONF_VALUE_num_items(conf->data), total_values + model.size()); } TEST(ConfTest, Parse) { diff --git a/crypto/conf/internal.h b/crypto/conf/internal.h index 1e39ac4985..04359ad25a 100644 --- a/crypto/conf/internal.h +++ b/crypto/conf/internal.h @@ -24,14 +24,10 @@ extern "C" { #endif -typedef struct conf_section_st CONF_SECTION; - -DEFINE_LHASH_OF(CONF_SECTION) DEFINE_LHASH_OF(CONF_VALUE) struct conf_st { - LHASH_OF(CONF_VALUE) *values; - LHASH_OF(CONF_SECTION) *sections; + LHASH_OF(CONF_VALUE) *data; }; // CONF_VALUE_new returns a freshly allocated and zeroed |CONF_VALUE|.