Always free clients list if we allocated it Closes #1130

diff --git a/src/main/client.c b/src/main/client.c
index f12650bd5737a251035f103ff85310134f878cde..e4691ed2bdf726ec47657c72572be72df435a509 100644 (file)
--- a/src/main/client.c
+++ b/src/main/client.c
@@ -527,10 +527,8 @@ static const CONF_PARSER client_config[] = {
        { NULL, -1, 0, NULL, NULL }
 };
 
-/*
- *     Create the linked list of clients from the new configuration
- *     type.  This way we don't have to change too much in the other
- *     source-files.
+/** Create the linked list of clients from the new configuration type
+ *
  */
 #ifdef WITH_TLS
 RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, bool tls_required)
@@ -540,8 +538,8 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
 {
        bool            global = false, in_server = false;
        CONF_SECTION    *cs;
-       RADCLIENT       *c;
-       RADCLIENT_LIST  *clients;
+       RADCLIENT       *c = NULL;
+       RADCLIENT_LIST  *clients = NULL;
 
        /*
         *      Be forgiving.  If there's already a clients, return
@@ -557,22 +555,12 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
 
        if (strcmp("server", cf_section_name1(section)) == 0) in_server = true;
 
-       /*
-        *      Associate the clients structure with the section.
-        */
-       if (cf_data_add(section, "clients", clients, NULL) < 0) {
-               cf_log_err_cs(section,
-                          "Failed to associate clients with section %s",
-                      cf_section_name1(section));
-               client_list_free(clients);
-               return NULL;
-       }
-
        for (cs = cf_subsection_find_next(section, NULL, "client");
-            cs != NULL;
+            cs;
             cs = cf_subsection_find_next(section, cs, "client")) {
                c = client_afrom_cs(cs, cs, in_server, false);
                if (!c) {
+                       client_list_free(clients);
                        return NULL;
                }
 
@@ -583,6 +571,7 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
                 */
                if (tls_required != c->tls_required) {
                        cf_log_err_cs(cs, "Client does not have the same TLS configuration as the listener");
+               error:
                        client_free(c);
                        client_list_free(clients);
                        return NULL;
@@ -597,12 +586,12 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
 #ifdef WITH_DYNAMIC_CLIENTS
 #ifdef HAVE_DIRENT_H
                if (c->client_server) {
-                       char const *value;
-                       CONF_PAIR *cp;
+                       char const      *value;
+                       CONF_PAIR       *cp;
                        DIR             *dir;
                        struct dirent   *dp;
-                       struct stat stat_buf;
-                       char buf2[2048];
+                       struct stat     stat_buf;
+                       char            buf2[2048];
 
                        /*
                         *      Find the directory where individual
@@ -613,10 +602,8 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
 
                        value = cf_pair_value(cp);
                        if (!value) {
-                               cf_log_err_cs(cs,
-                                          "The \"directory\" entry must not be empty");
-                               client_free(c);
-                               return NULL;
+                               cf_log_err_cs(cs, "The \"directory\" entry must not be empty");
+                               goto error;
                        }
 
                        DEBUG("including dynamic clients in %s", value);
@@ -624,8 +611,7 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
                        dir = opendir(value);
                        if (!dir) {
                                cf_log_err_cs(cs, "Error reading directory %s: %s", value, fr_syserror(errno));
-                               client_free(c);
-                               return NULL;
+                               goto error;
                        }
 
                        /*
@@ -645,33 +631,27 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
                                            isdigit((int)*p) ||
                                            (*p == ':') ||
                                            (*p == '.')) continue;
-                                               break;
+                                       break;
                                }
                                if (*p != '\0') continue;
 
-                               snprintf(buf2, sizeof(buf2), "%s/%s",
-                                        value, dp->d_name);
+                               snprintf(buf2, sizeof(buf2), "%s/%s", value, dp->d_name);
 
-                               if ((stat(buf2, &stat_buf) != 0) ||
-                                   S_ISDIR(stat_buf.st_mode)) continue;
+                               if ((stat(buf2, &stat_buf) != 0) || S_ISDIR(stat_buf.st_mode)) continue;
 
                                dc = client_read(buf2, in_server, true);
                                if (!dc) {
-                                       cf_log_err_cs(cs,
-                                                  "Failed reading client file \"%s\"", buf2);
-                                       client_free(c);
+                                       cf_log_err_cs(cs, "Failed reading client file \"%s\"", buf2);
                                        closedir(dir);
-                                       return NULL;
+                                       goto error;
                                }
 
                                /*
                                 *      Validate, and add to the list.
                                 */
                                if (!client_add_dynamic(clients, c, dc)) {
-
-                                       client_free(c);
                                        closedir(dir);
-                                       return NULL;
+                                       goto error;
                                }
                        } /* loop over the directory */
                        closedir(dir);
@@ -681,23 +661,27 @@ RADCLIENT_LIST *client_list_parse_section(CONF_SECTION *section, UNUSED bool tls
        add_client:
 #endif /* WITH_DYNAMIC_CLIENTS */
                if (!client_add(clients, c)) {
-                       cf_log_err_cs(cs,
-                                  "Failed to add client %s",
-                                  cf_section_name2(cs));
-                       client_free(c);
-                       return NULL;
+                       cf_log_err_cs(cs, "Failed to add client %s", cf_section_name2(cs));
+                       goto error;
                }
 
        }
 
+       /*
+        *      Associate the clients structure with the section.
+        */
+       if (cf_data_add(section, "clients", clients, NULL) < 0) {
+               cf_log_err_cs(section, "Failed to associate clients with section %s", cf_section_name1(section));
+               client_list_free(clients);
+               return NULL;
+       }
+
        /*
         *      Replace the global list of clients with the new one.
         *      The old one is still referenced from the original
         *      configuration, and will be freed when that is freed.
         */
-       if (global) {
-               root_clients = clients;
-       }
+       if (global) root_clients = clients;
 
        return clients;
 }