Merge pull request #54 from VCTLabs/valgrind-fixes

fix: usr: clean up memory leaks found with valgrind

inc/json.hh

@@ -36,16 +36,24 @@ class json {
         if (json_text) obj = json_tokener_parse(json_text);
         if (obj == NULL) throw json_parse_failure(json_text);
     }
+    // NOTE -- this constructor takes over ownership of raw json_object,
+    // do *NOT* manually zero out reference count on parameter object
+    // with json_object_put()
     explicit json(json_object *c_obj) : obj(c_obj) {
-        if (obj) json_object_get(obj);
-        else
-            obj = json_object_new_object();
+        if (obj == NULL) obj = json_object_new_object();
     }
     // release reference on underlying json_object*,
     // if this was last reference it will get freed
     ~json() { json_object_put(obj); }
 
-    json& operator=(const json &copy) { obj = copy.obj; json_object_get(obj); return *this; }
+    json& operator=(const json &copy) {
+        if (obj != NULL) {
+            json_object_put(obj);
+        }
+        obj = copy.obj;
+        json_object_get(obj);
+        return *this;
+    }
 
     bool operator==(const json &other) {
         bool is_same = json_object_equal(this->obj, other.obj);
@@ -154,11 +162,12 @@ class json {
         json_object_object_add(obj, field_name, bool_obj);
     }
 
-    // caller must make sure the 'value' object stays alive while it
-    // is still used as a field
     void set_field(const char *field_name, const json &value) {
         if (!json_object_is_type(obj, json_type_object))
             throw std::runtime_error("Not a hash-type object!");
+        // take extra reference so that value object will not be destroyed
+        // just because this object gets destroyed (add does not bump reference count)
+        json_object_get(value.obj);
         json_object_object_add(obj, field_name, value.obj);
     }
 

src/redis_ipc.c

@@ -177,6 +177,7 @@ void cleanup_per_thread_info(struct redis_ipc_per_thread *thread_info)
     safe_free(thread_info->component);
     safe_free(thread_info->thread);
     safe_free(thread_info->result_queue_path);
+    safe_free(thread_info->config.settings_writer);
     if (thread_info->redis_state)
     {
         // closes connection and frees memory related to this connection
@@ -187,6 +188,9 @@ void cleanup_per_thread_info(struct redis_ipc_per_thread *thread_info)
         // closes connection and frees memory related to this connection
         redisFree(thread_info->redis_sub_state);
     }
+
+    // finally free the top-level struct
+    safe_free(thread_info);
 }
 
 // initialize per-thread state
@@ -195,6 +199,7 @@ int redis_ipc_init(const char *this_component, const char *this_thread)
 {
     struct redis_ipc_per_thread *new_info = NULL;
     char result_queue_path[RIPC_MAX_IPC_PATH_LEN];
+    redisReply *reply = NULL;
     int ret = RIPC_FAIL;
 
     new_info = calloc(1, sizeof(struct redis_ipc_per_thread));
@@ -242,14 +247,18 @@ int redis_ipc_init(const char *this_component, const char *this_thread)
     redis_ipc_config_load();
 
     // enable HASH update events on redis server, to supprt settings notifications
-    redis_command("CONFIG SET notify-keyspace-events Kh");
+    reply = redis_command("CONFIG SET notify-keyspace-events Kh");
+    if (reply != NULL)
+    {
+        ret = RIPC_OK;
+        freeReplyObject(reply);
+    }
 
-    return RIPC_OK;
+    return ret;
 
 redis_ipc_init_failed:
     fprintf(stderr, "[ERROR] redis_ipc_init failed for thread %s\n", this_thread);
     cleanup_per_thread_info(new_info);
-    safe_free(new_info);
 
     return RIPC_FAIL;
 }
@@ -296,10 +305,11 @@ void redis_ipc_config_load()
     // which component is authorized to write settings (or all of them, if wildcard)
     // Note: intentionally not freeing lookup result, it becomes owned by config struct
     safe_free(thread_info->config.settings_writer);
-    thread_info->config.settings_writer = strdup(RIPC_DEFAULT_SETTINGS_WRITER);
     lookup = redis_read_hash_field(setting_hash_path, SETTINGS_WRITER_FIELD);
     if (lookup)
         thread_info->config.settings_writer = lookup;
+    else
+        thread_info->config.settings_writer = strdup(RIPC_DEFAULT_SETTINGS_WRITER);
 
     // clear flag to disable stderr debugging when looking up internal config settings
     thread_info->force_quiet = 0;

test/json_test.cpp

@@ -9,6 +9,7 @@ int main(int argc, char **argv)
     json_object *de = json_object_new_object();
 
     json snook(hum), took(de);
+    // snook and took should be holding references now, let go of original refs
     took.set_field("baggins", 9999);
     took.set_field("bilbo", "hungry");
     snook.set_field("betook", took);

test/settings_status_test.c

@@ -6,6 +6,7 @@ int main(int argc, char **argv)
 {
   json_object *setting = NULL;
   json_object *status = NULL;
+  char *field = NULL;
 
   redis_ipc_init("session", "main");
 
@@ -15,6 +16,7 @@ int main(int argc, char **argv)
   json_object_object_add(status, "procedure",
                          json_object_new_string("complicated"));
   redis_ipc_write_status(status);
+  json_object_put(status);
 
   setting = json_object_new_object();
   json_object_object_add(setting, "auto_finalize",
@@ -31,7 +33,8 @@ int main(int argc, char **argv)
   // should come back empty since above write failed
   setting = redis_ipc_read_setting("session");
   json_object_put(setting);
-  redis_ipc_read_setting_field("session", "location");
+  field = redis_ipc_read_setting_field("session", "location");
+  if (field) free(field);
 
   // authorize this component to write settings and try again
   redis_ipc_config_settings_writer("session");
@@ -63,12 +66,14 @@ int main(int argc, char **argv)
   setting = redis_ipc_read_setting("printer");
   json_object_put(setting);
 
-  setting = redis_ipc_config_stderr_debug(0);
+  redis_ipc_config_stderr_debug(0);
   fprintf(stderr, "** This attempt to read single setting should *not* print debug...\n");
-  redis_ipc_read_setting_field("printer", "paper_type");
-  setting = redis_ipc_config_stderr_debug(1);
+  field = redis_ipc_read_setting_field("printer", "paper_type");
+  if (field) free(field);
+  redis_ipc_config_stderr_debug(1);
   fprintf(stderr, "** This attempt to read single setting *should* print debug...\n");
-  redis_ipc_read_setting_field("printer", "paper_type");
+  field = redis_ipc_read_setting_field("printer", "paper_type");
+  if (field) free(field);
 
   redis_ipc_cleanup(getpid());