This commit fixes two memory leaks and adds a common exit function.
authorMichael Rash <mbr@cipherdyne.org>
Fri, 11 Nov 2011 03:33:32 +0000 (22:33 -0500)
committerMichael Rash <mbr@cipherdyne.org>
Fri, 11 Nov 2011 03:33:32 +0000 (22:33 -0500)
The two memory leaks were found with the test suite running in
--enable-valgrind mode - here are the relevant error messages:

For fwknopd server GPG clean up:

==345== 9 bytes in 1 blocks are definitely lost in loss record 2 of 2
==345==   at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==345==   by 0x52F6B81: strdup (strdup.c:43)
==345==   by 0x10FA57: add_string_list_ent (access.c:308)
==345==   by 0x110513: parse_access_file (access.c:387)
==345==   by 0x10B5FB: main (fwknopd.c:193)

For fwknop client rc file processing:

==8045== 568 bytes in 1 blocks are still reachable in loss record 12 of 12
==8045==    at 0x4C2815C: malloc (vg_replace_malloc.c:236)
==8045==    by 0x50A53AA: __fopen_internal (iofopen.c:76)
==8045==    by 0x10C3FF: process_rc (config_init.c:446)
==8045==    by 0x10C8F6: config_init (config_init.c:671)
==8045==    by 0x10AC9E: main (fwknop.c:62)

There is also a new clean_exit() function that makes it easier to ensure that
resources are deallocated upon existing.

15 files changed:
client/config_init.c
client/fwknop.c
client/fwknop_common.h
lib/fko_user.c
server/access.c
server/config_init.c
server/fw_util_ipf.c
server/fw_util_ipfw.c
server/fw_util_pf.c
server/fwknopd.c
server/fwknopd_common.h
server/incoming_spa.c
server/log_msg.c
server/pcap_capture.c
server/replay_cache.c

index f823160..7a5ab72 100644 (file)
@@ -453,7 +453,7 @@ process_rc(fko_cli_options_t *options)
         else
             fprintf(stderr, "Unable to open rc file: %s: %s\n",
                 rcfile, strerror(errno));
+
         return;
     }
 
@@ -548,6 +548,7 @@ process_rc(fko_cli_options_t *options)
         }
 
     } /* end while fgets rc */
+    fclose(rc);
 }
 
 /* Sanity and bounds checks for the various options.
index 0697555..22e6ed5 100644 (file)
@@ -361,9 +361,18 @@ main(int argc, char **argv)
 
     fko_destroy(ctx);
 
+    free_configs(&options);
+
     return(EXIT_SUCCESS);
 }
 
+void
+free_configs(fko_cli_options_t *opts)
+{
+    if (opts->resolve_url != NULL)
+        free(opts->resolve_url);
+}
+
 static int
 get_rand_port(fko_ctx_t ctx)
 {
index cd774c3..e0f3e30 100644 (file)
@@ -137,6 +137,8 @@ typedef struct fko_cli_options
 
 extern fko_cli_options_t options;
 
+void free_configs(fko_cli_options_t *opts);
+
 #endif /* FWKNOP_COMMON_H */
 
 /***EOF***/
index 6430343..8511e1a 100644 (file)
@@ -95,7 +95,6 @@ fko_set_username(fko_ctx_t ctx, const char *spoof_user)
     if(ctx->username == NULL)
         return(FKO_ERROR_MEMORY_ALLOCATION);
 
-
     return(FKO_SUCCESS);
 }
 
index 3eae40e..f5be4a0 100644 (file)
@@ -431,6 +431,7 @@ free_acc_string_list(acc_string_list_t *stl)
         last_stl = stl;
         stl = last_stl->next;
 
+        free(last_stl->str);
         free(last_stl);
     }
 }
@@ -571,7 +572,7 @@ acc_stanza_add(fko_srv_options_t *opts)
         log_msg(LOG_ERR,
             "Fatal memory allocation error adding access stanza"
         );
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* If this is not the first acc entry, we walk our acc pointer to the
@@ -664,7 +665,7 @@ parse_access_file(fko_srv_options_t *opts)
         fprintf(stderr, "[*] Access file: '%s' was not found.\n",
             opts->config[CONF_ACCESS_FILE]);
 
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     if ((file_ptr = fopen(opts->config[CONF_ACCESS_FILE], "r")) == NULL)
@@ -673,7 +674,7 @@ parse_access_file(fko_srv_options_t *opts)
             opts->config[CONF_ACCESS_FILE]);
         perror(NULL);
 
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Initialize the access list.
@@ -733,7 +734,7 @@ parse_access_file(fko_srv_options_t *opts)
                     fprintf(stderr,
                         "[*] Data error in access file: '%s'\n",
                         opts->config[CONF_ACCESS_FILE]);
-                    exit(EXIT_FAILURE);
+                    clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
                 }
             }
 
@@ -766,7 +767,7 @@ parse_access_file(fko_srv_options_t *opts)
                 fprintf(stderr,
                     "[*] KEY value is not properly set in stanza source '%s' in access file: '%s'\n",
                     curr_acc->source, opts->config[CONF_ACCESS_FILE]);
-                exit(EXIT_FAILURE);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
             add_acc_string(&(curr_acc->key), val);
         }
@@ -789,7 +790,7 @@ parse_access_file(fko_srv_options_t *opts)
             {
                 fprintf(stderr, "Unable to determine UID for CMD_EXEC_USER: %s.\n",
                     errno ? strerror(errno) : "Not a user on this system");
-                exit(EXIT_FAILURE);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
 
             curr_acc->cmd_exec_uid = pw->pw_uid;
@@ -813,7 +814,7 @@ parse_access_file(fko_srv_options_t *opts)
                 fprintf(stderr,
                     "[*] GPG_HOME_DIR directory '%s' stat()/existence problem in stanza source '%s' in access file: '%s'\n",
                     val, curr_acc->source, opts->config[CONF_ACCESS_FILE]);
-                exit(EXIT_FAILURE);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
         }
         else if(CONF_VAR_IS(var, "GPG_DECRYPT_ID"))
@@ -827,7 +828,7 @@ parse_access_file(fko_srv_options_t *opts)
                 fprintf(stderr,
                     "[*] GPG_DECRYPT_PW value is not properly set in stanza source '%s' in access file: '%s'\n",
                     curr_acc->source, opts->config[CONF_ACCESS_FILE]);
-                exit(EXIT_FAILURE);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
             add_acc_string(&(curr_acc->gpg_decrypt_pw), val);
         }
@@ -863,7 +864,7 @@ parse_access_file(fko_srv_options_t *opts)
         fprintf(stderr,
             "[*] Could not find valid SOURCE stanza in access file: '%s'\n",
             opts->config[CONF_ACCESS_FILE]);
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Sanity check the last stanza
@@ -873,7 +874,7 @@ parse_access_file(fko_srv_options_t *opts)
         fprintf(stderr,
             "[*] Data error in access file: '%s'\n",
             opts->config[CONF_ACCESS_FILE]);
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Expand our the expandable fields into their respective data buckets.
index 349f486..3a5069e 100644 (file)
@@ -48,7 +48,7 @@ set_config_entry(fko_srv_options_t *opts, const int var_ndx, const char *value)
     if(var_ndx < 0 || var_ndx >= NUMBER_OF_CONFIG_ENTRIES)
     {
         fprintf(stderr, "Index value of %i is not valid\n", var_ndx);
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* If this particular entry was already set (i.e. not NULL), then
@@ -74,7 +74,7 @@ set_config_entry(fko_srv_options_t *opts, const int var_ndx, const char *value)
     if(opts->config[var_ndx] == NULL)
     {
         fprintf(stderr, "*Fatal memory allocation error!\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     strlcpy(opts->config[var_ndx], value, space_needed);
@@ -135,8 +135,7 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
     {
         fprintf(stderr, "[*] Config file: '%s' was not found.\n",
             config_file);
-
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     if ((cfile_ptr = fopen(config_file, "r")) == NULL)
@@ -145,7 +144,7 @@ parse_config_file(fko_srv_options_t *opts, const char *config_file)
             config_file);
         perror(NULL);
 
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     while ((fgets(conf_line_buf, MAX_LINE_LEN, cfile_ptr)) != NULL)
@@ -498,7 +497,7 @@ validate_options(fko_srv_options_t *opts)
         fprintf(stderr,
             "The -D, -K, -R, and -S options are mutually exclusive.  Pick only one.\n"
         );
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     if(opts->config[CONF_FIREWALL_EXE] == NULL)
@@ -506,7 +505,7 @@ validate_options(fko_srv_options_t *opts)
         fprintf(stderr,
             "No firewall command executable is set. Please check FIREWALL_EXE in fwknopd.conf.\n"
         );
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     return;
@@ -566,7 +565,7 @@ config_init(fko_srv_options_t *opts, int argc, char **argv)
         switch(cmd_arg) {
             case 'h':
                 usage();
-                exit(EXIT_SUCCESS);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_SUCCESS);
                 break;
 
         /* Look for configuration file arg.
@@ -692,7 +691,7 @@ config_init(fko_srv_options_t *opts, int argc, char **argv)
                     fprintf(stderr,
                         "[*] Directory '%s' could not stat()/does not exist?\n",
                         optarg);
-                    exit(EXIT_FAILURE);
+                    clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
                 }
                 break;
             case 'i':
@@ -727,11 +726,11 @@ config_init(fko_srv_options_t *opts, int argc, char **argv)
                 break;
             case 'V':
                 fprintf(stdout, "fwknopd server %s\n", MY_VERSION);
-                exit(EXIT_SUCCESS);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_SUCCESS);
                 break;
             default:
                 usage();
-                exit(EXIT_FAILURE);
+                clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
         }
     }
 
index 56a7ea5..7233749 100644 (file)
@@ -91,7 +91,7 @@ fw_initialize(const fko_srv_options_t *opts)
     if(res != 0)
     {
         fprintf(stderr, "Warning: Errors detected during fwknop custom chain creation.\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 }
 
index 30522a8..f82def0 100644 (file)
@@ -217,7 +217,7 @@ fw_initialize(const fko_srv_options_t *opts)
     if(res != 0)
     {
         fprintf(stderr, "Fatal: Errors detected during ipfw rules initialization.\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Allocate our rule_map array for tracking active (and expired) rules.
@@ -227,7 +227,7 @@ fw_initialize(const fko_srv_options_t *opts)
     if(fwc.rule_map == NULL)
     {
         fprintf(stderr, "Fatal: Memory allocation error in fw_initialize.\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Create a check-state rule if necessary.
index 0fe2ce6..22e32d0 100644 (file)
@@ -182,7 +182,7 @@ fw_initialize(const fko_srv_options_t *opts)
     if (! anchor_active(opts))
     {
         fprintf(stderr, "Warning: the fwknop anchor is not active in the pf policy\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Delete any existing rules in the fwknop anchor
index efe857b..337ac0e 100644 (file)
@@ -78,18 +78,18 @@ main(int argc, char **argv)
                 if(res == 0)
                 {
                     fprintf(stdout, "Killed fwknopd (pid=%i)\n", old_pid);
-                    exit(EXIT_SUCCESS);
+                    clean_exit(&opts, NO_FW_CLEANUP, EXIT_SUCCESS);
                 }
                 else
                 {
                     perror("Unable to kill fwknop: ");
-                    exit(EXIT_FAILURE);
+                    clean_exit(&opts, NO_FW_CLEANUP, EXIT_FAILURE);
                 }
             }
             else
             {
                 fprintf(stderr, "No running fwknopd detected.\n");
-                exit(EXIT_FAILURE);
+                clean_exit(&opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
         }
 
@@ -104,7 +104,7 @@ main(int argc, char **argv)
             else
                 fprintf(stdout, "No running fwknopd detected.\n");
 
-            exit(EXIT_SUCCESS);
+            clean_exit(&opts, NO_FW_CLEANUP, EXIT_SUCCESS);
         }
 
         /* Restart the currently running fwknopd?
@@ -119,18 +119,18 @@ main(int argc, char **argv)
                 if(res == 0)
                 {
                     fprintf(stdout, "Sent restart signal to fwknopd (pid=%i)\n", old_pid);
-                    exit(EXIT_SUCCESS);
+                    clean_exit(&opts, NO_FW_CLEANUP, EXIT_SUCCESS);
                 }
                 else
                 {
                     perror("Unable to send signal to fwknop: ");
-                    exit(EXIT_FAILURE);
+                    clean_exit(&opts, NO_FW_CLEANUP, EXIT_FAILURE);
                 }
             }
             else
             {
                 fprintf(stdout, "No running fwknopd detected.\n");
-                exit(EXIT_FAILURE);
+                clean_exit(&opts, NO_FW_CLEANUP, EXIT_FAILURE);
             }
         }
 
@@ -179,14 +179,13 @@ main(int argc, char **argv)
         if(opts.fw_list == 1 || opts.fw_list_all == 1)
         {
             fw_dump_rules(&opts);
-            exit(EXIT_SUCCESS);
+            clean_exit(&opts, NO_FW_CLEANUP, EXIT_SUCCESS);
         }
 
         if(opts.fw_flush == 1)
         {
             fprintf(stdout, "Deleting any existing firewall rules...\n");
-            fw_cleanup(&opts);
-            exit(EXIT_SUCCESS);
+            clean_exit(&opts, FW_CLEANUP, EXIT_SUCCESS);
         }
 
         /* Process the access.conf file.
@@ -200,7 +199,7 @@ main(int argc, char **argv)
         {
             dump_config(&opts);
             dump_access_list(&opts);
-            exit(EXIT_SUCCESS);
+            clean_exit(&opts, NO_FW_CLEANUP, EXIT_SUCCESS);
         }
 
         /* If we are a new process (just being started), proceed with normal
@@ -225,7 +224,7 @@ main(int argc, char **argv)
                         "* An instance of fwknopd is already running: (PID=%i).\n", old_pid
                     );
 
-                    exit(EXIT_FAILURE);
+                    clean_exit(&opts, NO_FW_CLEANUP, EXIT_FAILURE);
                 }
                 else if(old_pid < 0)
                 {
@@ -681,4 +680,19 @@ get_running_pid(const fko_srv_options_t *opts)
     return(rpid);
 }
 
+void
+clean_exit(fko_srv_options_t *opts, unsigned int fw_cleanup_flag, unsigned int exit_status)
+{
+    if(fw_cleanup_flag == FW_CLEANUP)
+        fw_cleanup(opts);
+
+#if USE_FILE_CACHE
+    free_replay_list(opts);
+#endif
+
+    free_logging();
+    free_configs(opts);
+    exit(exit_status);
+}
+
 /***EOF***/
index e26a2fb..fbc5a83 100644 (file)
@@ -444,6 +444,12 @@ typedef struct fko_srv_options
 
 extern fko_srv_options_t options;
 
+/* For cleaning up memory before exiting
+*/
+#define FW_CLEANUP          1
+#define NO_FW_CLEANUP       0
+void clean_exit(fko_srv_options_t *opts, unsigned int fw_cleanup_flag, unsigned int exit_status);
+
 #endif /* FWKNOPD_COMMON_H */
 
 /***EOF***/
index 7e65549..f96ab28 100644 (file)
@@ -279,6 +279,7 @@ incoming_spa(fko_srv_options_t *opts)
             /* Now decrypt the data.
             */
             res = fko_decrypt_spa_data(ctx, acc->gpg_decrypt_pw);
+
         }
         else
         {
index f680c2a..0f3861f 100644 (file)
@@ -87,7 +87,7 @@ init_logging(fko_srv_options_t *opts) {
     if(log_name == NULL)
     {
         fprintf(stderr, "Memory allocation error setting log_name!\n");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Set our name.
index ebdbc93..7936514 100644 (file)
@@ -78,13 +78,13 @@ pcap_capture(fko_srv_options_t *opts)
     if(pcap == NULL)
     {
         log_msg(LOG_ERR, "[*] pcap_open_live error: %s\n", errstr);
-        exit(EXIT_FAILURE);
+        clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
     }
 
     if (pcap == NULL)
     {
         log_msg(LOG_ERR, "[*] pcap error: %s", errstr);
-        exit(EXIT_FAILURE);
+        clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Set pcap filters, if any.
@@ -96,7 +96,7 @@ pcap_capture(fko_srv_options_t *opts)
             log_msg(LOG_ERR, "[*] Error compiling pcap filter: %s",
                 pcap_geterr(pcap)
             );
-            exit(EXIT_FAILURE);
+            clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
         }
 
         if(pcap_setfilter(pcap, &fp) == -1)
@@ -104,7 +104,7 @@ pcap_capture(fko_srv_options_t *opts)
             log_msg(LOG_ERR, "[*] Error setting pcap filter: %s",
                 pcap_geterr(pcap)
             );
-            exit(EXIT_FAILURE);
+            clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
         }
 
         log_msg(LOG_INFO, "PCAP filter is: %s", opts->config[CONF_PCAP_FILTER]);
@@ -154,7 +154,7 @@ pcap_capture(fko_srv_options_t *opts)
         log_msg(LOG_ERR, "[*] Error setting pcap nonblocking to %i: %s",
             0, errstr
         );
-        exit(EXIT_FAILURE);
+        clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* Initialize our signal handlers. You can check the return value for
@@ -257,7 +257,7 @@ pcap_capture(fko_srv_options_t *opts)
                 log_msg(LOG_ERR, "[*] %i consecutive pcap errors.  Giving up",
                     pcap_errcnt
                 );
-                exit(EXIT_FAILURE);
+                clean_exit(opts, FW_CLEANUP, EXIT_FAILURE);
             }
         }
         else if(pending_break == 1 || res == -2)
index 04993de..77566af 100644 (file)
@@ -99,7 +99,7 @@ rotate_digest_cache_file(fko_srv_options_t *opts)
     if(new_file == NULL)
     {
         log_msg(LOG_ERR, "rotate_digest_cache_file: Memory allocation error.");
-        exit(EXIT_FAILURE);
+        clean_exit(opts, NO_FW_CLEANUP, EXIT_FAILURE);
     }
 
     /* The new filename is just the original with a trailing '-old'.