From 6fadb50dba9bbade43f8760656d80437abeae958 Mon Sep 17 00:00:00 2001
From: samuel40791765 <sachiang@amazon.com>
Date: Tue, 12 Nov 2024 23:03:41 +0000
Subject: [PATCH] Revert "Replace CONF's internal representation with something
 more typesafe"

This reverts commit cc932cf394d2af845fdc46eac14f21886d5c98c9.
---
 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|.