Skip to content

Commit

Permalink
Revert "Replace CONF's internal representation with something more ty…
Browse files Browse the repository at this point in the history 
…pesafe" (aws#1986)

This reverts commit cc932cf.

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.
  • Loading branch information
@samuel40791765
samuel40791765 authored Nov 14, 2024
1 parent 13f467e commit f0c9b3a
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 93 deletions.
174 changes: 88 additions & 86 deletions crypto/conf/conf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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);
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}

Expand Down
3 changes: 1 addition & 2 deletions crypto/conf/conf_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 1 addition & 5 deletions crypto/conf/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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|.
Expand Down

0 comments on commit f0c9b3a

Please sign in to comment.