From 4394b6859d32e6c100148c25c9a8fe57d4ce208c Mon Sep 17 00:00:00 2001
From: Ben Kibbey <bjk@luxsci.net>
Date: Tue, 14 Mar 2023 20:36:17 -0700
Subject: [PATCH] cache: Fix memory leaks.

When re-opening a new data file the refcount wasn't getting decremented.

Also, in the case that clear_once() fails, keep the cache entry until
the cache data can be cleared which may occur during the next
cache_adjust_timeout() interval rather than freeing the cache entry but
not the data.
---
 src/cache.c    | 33 +++++++++++++++++++++++++--------
 src/cache.h    |  3 ++-
 src/commands.c |  8 ++++----
 src/pwmd.c     |  2 +-
 4 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/src/cache.c b/src/cache.c
index 8f81bef3..6484953c 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -101,7 +101,7 @@ cache_lock_mutex (void *ctx, const char *filename, long lock_timeout, int add,
     }
   else if (add)
     {
-      rc = cache_add_file (filename, NULL, timeout);
+      rc = cache_add_file (filename, NULL, timeout, 0);
       if (!rc)
 	{
 	  p = get_entry (filename);
@@ -129,9 +129,17 @@ remove_entry (const char *filename)
       /* Keep a refcount because another client may be editing this same new
        * file. The entry needs to be kept in case the other client SAVE's.
        */
-      p->refcount--;
-      if (!p->refcount)
+      if (!(p->refcount-1))
 	free_entry (p, 0);
+      else
+        {
+          p->refcount--;
+          if ((p->flags & CACHE_FLAG_LOCKED))
+            {
+              p->flags &= ~CACHE_FLAG_LOCKED;
+              MUTEX_UNLOCK (p->mutex);
+            }
+        }
     }
 
   pthread_cleanup_pop (1);
@@ -238,11 +246,13 @@ cache_unlock_mutex (const char *filename, int remove)
 
   if (p)
     {
-      p->flags &= ~CACHE_FLAG_LOCKED;
       if (remove)
 	remove_entry (filename);
       else
-        MUTEX_UNLOCK (p->mutex);
+        {
+          p->flags &= ~CACHE_FLAG_LOCKED;
+          MUTEX_UNLOCK (p->mutex);
+        }
     }
 
   pthread_cleanup_pop (1);
@@ -635,7 +645,8 @@ cache_get_data (const char *filename, int *defer)
 }
 
 gpg_error_t
-cache_add_file (const char *filename, struct cache_data_s *data, long timeout)
+cache_add_file (const char *filename, struct cache_data_s *data, long timeout,
+                int same_file)
 {
   MUTEX_LOCK (&cache_mutex);
   file_cache_t *p = get_entry (filename);
@@ -662,7 +673,10 @@ cache_add_file (const char *filename, struct cache_data_s *data, long timeout)
       if (!rc)
         {
           p->data = data;
-          p->refcount++;
+
+          if (!same_file)
+            p->refcount++;
+
           rc = set_timeout (filename, timeout, p->defer_clear, 0);
         }
     }
@@ -781,7 +795,10 @@ free_entry (file_cache_t *p, int agent)
 
   rc = clear_once (p, agent, 0);
   if (rc)
-    log_write ("%s(): %s", __FUNCTION__, pwmd_strerror (rc));
+    {
+      log_write ("%s(): %s", __FUNCTION__, pwmd_strerror (rc));
+      return;
+    }
 
   key_cache = slist_remove (key_cache, p);
 
diff --git a/src/cache.h b/src/cache.h
index 449d6830..715e594e 100644
--- a/src/cache.h
+++ b/src/cache.h
@@ -60,7 +60,8 @@ void cache_adjust_timeout ();
 gpg_error_t cache_set_timeout (const char *, long timeout);
 gpg_error_t cache_iscached (const char *, int *defer, int agent, int sign);
 gpg_error_t cache_clear (struct client_s *, const char *, int agent, int force);
-gpg_error_t cache_add_file (const char *, struct cache_data_s *, long timeout);
+gpg_error_t cache_add_file (const char *, struct cache_data_s *, long timeout,
+                            int same_file);
 void cache_deinit ();
 gpg_error_t cache_init ();
 void cache_mutex_init ();
diff --git a/src/commands.c b/src/commands.c
index 0aa980ee..d36c761d 100644
--- a/src/commands.c
+++ b/src/commands.c
@@ -154,7 +154,7 @@ unlock_file_mutex (struct client_s *client, int remove)
   if (client->flags & FLAG_KEEP_LOCK && client->flags & FLAG_HAS_LOCK)
     return 0;
 
-  if (!(client->flags & FLAG_HAS_LOCK))
+  if (!remove && !(client->flags & FLAG_HAS_LOCK))
     return GPG_ERR_NOT_LOCKED;
 
   rc = cache_unlock_mutex (client->filename, remove);
@@ -277,8 +277,8 @@ reset_client (struct client_s *client)
   int flock_fd = client->flock_fd;
   struct bulk_cmd_s *bulk_p = client->bulk_p;
 
-  unlock_file_mutex (client, client->flags & FLAG_NEW);
   cache_lock ();
+  unlock_file_mutex (client, client->flags & FLAG_NEW);
   free_client (client);
   memset (client, 0, sizeof (struct client_s));
   client->flock_fd = flock_fd;
@@ -681,7 +681,7 @@ done:
                     rc = send_status (ctx, STATUS_CACHE, NULL);
                 }
               else
-                rc = cache_add_file (client->filename, cdata, timeout);
+                rc = cache_add_file (client->filename, cdata, timeout, same_file);
 
               if (!rc)
                 cache_plaintext_set (client->filename, client->doc, 0);
@@ -1302,7 +1302,7 @@ save_command (assuan_context_t ctx, char *line)
                   if (!cache_rc) // wont increment refcount
                     rc = cache_set_data (client->filename, cdata);
                   else
-                    rc = cache_add_file (client->filename, cdata, timeout);
+                    rc = cache_add_file (client->filename, cdata, timeout, 0);
                 }
 
               if (!rc && (cache_rc || (client->flags & FLAG_NEW)))
diff --git a/src/pwmd.c b/src/pwmd.c
index e19d6f9f..63fb5287 100644
--- a/src/pwmd.c
+++ b/src/pwmd.c
@@ -1188,7 +1188,7 @@ do_cache_push (struct crypto_s *crypto)
     }
 
   long timeout = config_get_long (crypto->filename, "cache_timeout");
-  rc = cache_add_file (crypto->filename, cdata, timeout);
+  rc = cache_add_file (crypto->filename, cdata, timeout, 0);
   return rc;
 }
 
-- 
2.11.4.GIT