Drop ossl_namemap_add_name_n() and simplify ossl_namemap_add_names()

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18341)

crypto/core_namemap.c

@@ -166,6 +166,7 @@ int ossl_namemap_doall_names(const OSSL_NAMEMAP *namemap, int number,
     return 1;
 }
 
+/* This function is not thread safe, the namemap must be locked */
 static int namemap_name2num(const OSSL_NAMEMAP *namemap,
                             const char *name)
 {
@@ -239,25 +240,22 @@ const char *ossl_namemap_num2name(const OSSL_NAMEMAP *namemap, int number,
     return data.name;
 }
 
-static int namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
+/* This function is not thread safe, the namemap must be locked */
-                              const char *name, size_t name_len)
+static int namemap_add_name(OSSL_NAMEMAP *namemap, int number,
+                            const char *name)
 {
     NAMENUM_ENTRY *namenum = NULL;
     int tmp_number;
-    char *tmp = OPENSSL_strndup(name, name_len);
-
-    if (tmp == NULL)
-        return 0;
 
     /* If it already exists, we don't add it */
-    if ((tmp_number = namemap_name2num(namemap, tmp)) != 0) {
+    if ((tmp_number = namemap_name2num(namemap, name)) != 0)
-        OPENSSL_free(tmp);
         return tmp_number;
-    }
 
     if ((namenum = OPENSSL_zalloc(sizeof(*namenum))) == NULL)
+        return 0;
+
+    if ((namenum->name = OPENSSL_strdup(name)) == NULL)
         goto err;
-    namenum->name = tmp;
 
     /* The tsan_counter use here is safe since we're under lock */
     namenum->number =
@@ -269,13 +267,12 @@ static int namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
     return namenum->number;
 
  err:
-    OPENSSL_free(tmp);
+    namenum_free(namenum);
-    OPENSSL_free(namenum);
     return 0;
 }
 
-int ossl_namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
+int ossl_namemap_add_name(OSSL_NAMEMAP *namemap, int number,
-                            const char *name, size_t name_len)
+                          const char *name)
 {
     int tmp_number;
 
@@ -284,29 +281,20 @@ int ossl_namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
         namemap = ossl_namemap_stored(NULL);
 #endif
 
-    if (name == NULL || name_len == 0 || namemap == NULL)
+    if (name == NULL || *name == 0 || namemap == NULL)
         return 0;
 
     if (!CRYPTO_THREAD_write_lock(namemap->lock))
         return 0;
-    tmp_number = namemap_add_name_n(namemap, number, name, name_len);
+    tmp_number = namemap_add_name(namemap, number, name);
     CRYPTO_THREAD_unlock(namemap->lock);
     return tmp_number;
 }
 
-int ossl_namemap_add_name(OSSL_NAMEMAP *namemap, int number, const char *name)
-{
-    if (name == NULL)
-        return 0;
-
-    return ossl_namemap_add_name_n(namemap, number, name, strlen(name));
-}
-
 int ossl_namemap_add_names(OSSL_NAMEMAP *namemap, int number,
                            const char *names, const char separator)
 {
-    const char *p, *q;
+    char *tmp, *p, *q, *endp;
-    size_t l;
 
     /* Check that we have a namemap */
     if (!ossl_assert(namemap != NULL)) {
@@ -314,71 +302,71 @@ int ossl_namemap_add_names(OSSL_NAMEMAP *namemap, int number,
         return 0;
     }
 
-    if (!CRYPTO_THREAD_write_lock(namemap->lock))
+    if ((tmp = OPENSSL_strdup(names)) == NULL)
         return 0;
+
+    if (!CRYPTO_THREAD_write_lock(namemap->lock)) {
+        OPENSSL_free(tmp);
+        return 0;
+    }
     /*
      * Check that no name is an empty string, and that all names have at
      * most one numeric identity together.
      */
-    for (p = names; *p != '\0'; p = (q == NULL ? p + l : q + 1)) {
+    for (p = tmp; *p != '\0'; p = q) {
         int this_number;
-        char *allocated;
+        size_t l;
-        const char *tmp;
 
         if ((q = strchr(p, separator)) == NULL) {
             l = strlen(p);       /* offset to \0 */
-            tmp = p;
+            q = p + l;
-            allocated = NULL;
         } else {
             l = q - p;           /* offset to the next separator */
-            tmp = allocated = OPENSSL_strndup(p, l);
+            *q++ = '\0';
-            if (tmp == NULL)
-                goto err;
         }
 
-        this_number = namemap_name2num(namemap, tmp);
+        if (*p == '\0') {
-        OPENSSL_free(allocated);
-
-        if (*p == '\0' || *p == separator) {
             ERR_raise(ERR_LIB_CRYPTO, CRYPTO_R_BAD_ALGORITHM_NAME);
-            goto err;
+            number = 0;
+            goto end;
         }
+
+        this_number = namemap_name2num(namemap, p);
+
         if (number == 0) {
             number = this_number;
         } else if (this_number != 0 && this_number != number) {
             ERR_raise_data(ERR_LIB_CRYPTO, CRYPTO_R_CONFLICTING_NAMES,
-                           "\"%.*s\" has an existing different identity %d (from \"%s\")",
+                           "\"%s\" has an existing different identity %d (from \"%s\")",
-                           l, p, this_number, names);
+                           p, this_number, names);
-            goto err;
+            number = 0;
+            goto end;
         }
     }
+    endp = p;
 
     /* Now that we have checked, register all names */
-    for (p = names; *p != '\0'; p = (q == NULL ? p + l : q + 1)) {
+    for (p = tmp; p < endp; p = q) {
         int this_number;
 
-        if ((q = strchr(p, separator)) == NULL)
+        q = p + strlen(p) + 1;
-            l = strlen(p);       /* offset to \0 */
-        else
-            l = q - p;           /* offset to the next separator */
 
-        this_number = namemap_add_name_n(namemap, number, p, l);
+        this_number = namemap_add_name(namemap, number, p);
         if (number == 0) {
             number = this_number;
         } else if (this_number != number) {
             ERR_raise_data(ERR_LIB_CRYPTO, ERR_R_INTERNAL_ERROR,
                            "Got number %d when expecting %d",
                            this_number, number);
-            goto err;
+            number = 0;
+            goto end;
         }
     }
 
+ end:
     CRYPTO_THREAD_unlock(namemap->lock);
+    OPENSSL_free(tmp);
     return number;
-
- err:
-    CRYPTO_THREAD_unlock(namemap->lock);
-    return 0;
 }
 
 /*-

doc/internal/man3/ossl_namemap_new.pod

@@ -3,7 +3,7 @@
 =head1 NAME
 
 ossl_namemap_new, ossl_namemap_free, ossl_namemap_stored, ossl_namemap_empty,
-ossl_namemap_add_name, ossl_namemap_add_name_n, ossl_namemap_add_names,
+ossl_namemap_add_name, ossl_namemap_add_names,
 ossl_namemap_name2num, ossl_namemap_name2num_n,
 ossl_namemap_doall_names
 - internal number E<lt>-E<gt> name map
@@ -19,8 +19,6 @@ ossl_namemap_doall_names
  int ossl_namemap_empty(OSSL_NAMEMAP *namemap);
 
  int ossl_namemap_add_name(OSSL_NAMEMAP *namemap, int number, const char *name);
- int ossl_namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
-                             const char *name, size_t name_len);
 
  int ossl_namemap_name2num(const OSSL_NAMEMAP *namemap, const char *name);
  int ossl_namemap_name2num_n(const OSSL_NAMEMAP *namemap,
@@ -62,10 +60,9 @@ names already associated with that number.
 ossl_namemap_name2num() finds the number corresponding to the given
 I<name>.
 
-ossl_namemap_add_name_n() and ossl_namemap_name2num_n() do the same thing
+ossl_namemap_name2num_n() does the same thing as
-as ossl_namemap_add_name() and ossl_namemap_name2num(), but take a string
+ossl_namemap_name2num(), but takes a string length I<name_len> as well,
-length I<name_len> as well, allowing the caller to use a fragment of
+allowing the caller to use a fragment of a string as a name.
-a string as a name.
 
 ossl_namemap_doall_names() walks through all names associated with
 I<number> in the given I<namemap> and calls the function I<fn> for
@@ -88,8 +85,8 @@ ossl_namemap_empty() returns 1 if the B<OSSL_NAMEMAP> is NULL or
 empty, 0 if it's not empty, or -1 on internal error (such as inability
 to lock).
 
-ossl_namemap_add_name() and ossl_namemap_add_name_n() return the number
+ossl_namemap_add_name() returns the number associated with the added
-associated with the added string, or zero on error.
+string, or zero on error.
 
 ossl_namemap_num2names() returns a pointer to a NULL-terminated list of
 pointers to the names corresponding to the given number, or NULL if

include/internal/namemap.h

@@ -18,8 +18,6 @@ void ossl_namemap_free(OSSL_NAMEMAP *namemap);
 int ossl_namemap_empty(OSSL_NAMEMAP *namemap);
 
 int ossl_namemap_add_name(OSSL_NAMEMAP *namemap, int number, const char *name);
-int ossl_namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
-                            const char *name, size_t name_len);
 
 /*
  * The number<->name relationship is 1<->many