PR26125: debuginfod client cache - rmdir harder

With PR25365, we accidentally lost the ability to rmdir client-cache
directories corresponding to buildids.  Bring this back, with some
attention to a possible race between a client doing cleanup and
another client doing lookups at the same time.

Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

4 files changed, 43 insertions, 20 deletions

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index ad9dbc0a..9af641ec 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,11 @@
+2021-04-26  Frank Ch. Eigler <fche@redhat.com>
+
+	PR26125
+	* debuginfod-client.c (debuginfod_clean_cache): For directory
+	rmdir, check mtime first.
+	(debuginfod_query_server): Try mkdir / mkstemp up to twice,
+	in case of race.
+
 2021-04-23  Frank Ch. Eigler <fche@redhat.com>
 
 	PR27701
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 7fdc6c9f..0170500f 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -307,12 +307,13 @@ debuginfod_clean_cache(debuginfod_client *c,
     return -errno;
 
   regex_t re;
-  const char * pattern = ".*/[a-f0-9]+/(debuginfo|executable|source.*)$";
+  const char * pattern = ".*/[a-f0-9]+(/debuginfo|/executable|/source.*|)$"; /* include dirs */
   if (regcomp (&re, pattern, REG_EXTENDED | REG_NOSUB) != 0)
     return -ENOMEM;
 
   FTSENT *f;
   long files = 0;
+  time_t now = time(NULL);
   while ((f = fts_read(fts)) != NULL)
     {
       /* ignore any files that do not match the pattern.  */
@@ -327,15 +328,16 @@ debuginfod_clean_cache(debuginfod_client *c,
       switch (f->fts_info)
         {
         case FTS_F:
-          /* delete file if max_unused_age has been met or exceeded.  */
-          /* XXX consider extra effort to clean up old tmp files */
-          if (time(NULL) - f->fts_statp->st_atime >= max_unused_age)
-            unlink (f->fts_path);
+          /* delete file if max_unused_age has been met or exceeded w.r.t. atime.  */
+          if (now - f->fts_statp->st_atime >= max_unused_age)
+            (void) unlink (f->fts_path);
           break;
 
         case FTS_DP:
-          /* Remove if empty. */
-          (void) rmdir (f->fts_path);
+          /* Remove if old & empty.  Weaken race against concurrent creation by 
+             checking mtime. */
+          if (now - f->fts_statp->st_mtime >= max_unused_age)
+            (void) rmdir (f->fts_path);
           break;
 
         default:
@@ -715,19 +717,18 @@ debuginfod_query_server (debuginfod_client *c,
     }
   /* thereafter, goto out0 on error*/
 
-  /* create target directory in cache if not found.  */
-  struct stat st;
-  if (stat(target_cache_dir, &st) == -1 && mkdir(target_cache_dir, 0700) < 0)
-    {
-      rc = -errno;
-      goto out0;
-    }
-
-  /* NB: write to a temporary file first, to avoid race condition of
-     multiple clients checking the cache, while a partially-written or empty
-     file is in there, being written from libcurl. */
-  fd = mkstemp (target_cache_tmppath);
-  if (fd < 0)
+  /* Because of a race with cache cleanup / rmdir, try to mkdir/mkstemp up to twice. */
+  for(int i=0; i<2; i++) {
+    /* (re)create target directory in cache */
+    (void) mkdir(target_cache_dir, 0700);
+
+    /* NB: write to a temporary file first, to avoid race condition of
+       multiple clients checking the cache, while a partially-written or empty
+       file is in there, being written from libcurl. */
+    fd = mkstemp (target_cache_tmppath);
+    if (fd >= 0) break;
+  }
+  if (fd < 0) /* Still failed after two iterations. */
     {
       rc = -errno;
       goto out0;
diff --git a/tests/ChangeLog b/tests/ChangeLog
index eac006d0..0d2c5edd 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -8,6 +8,11 @@
 	(EXTRA_DIST): Add run-low_high_pc-dw-form-indirect.sh,
 	run-readelf-dw-form-indirect.sh, and testfile-dw-form-indirect.bz2.
 
+2021-04-26  Frank Ch. Eigler <fche@redhat.com>
+
+	PR26125
+	* run-debuginfod-find.sh: Add test case for cache cleanup rmdir.
+
 2021-04-23  Frank Ch. Eigler  <fche@redhat.com>
 
 	* run-debuginfod-find.sh: Add a tiny test for client object reuse.
diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh
index 11e84983..3b9a5a6e 100755
--- a/tests/run-debuginfod-find.sh
+++ b/tests/run-debuginfod-find.sh
@@ -571,6 +571,10 @@ mkdir $DEBUGINFOD_CACHE_PATH/malformed
 touch $DEBUGINFOD_CACHE_PATH/malformed0
 touch $DEBUGINFOD_CACHE_PATH/malformed/malformed1
 
+# A valid format for an empty buildid subdirectory
+mkdir $DEBUGINFOD_CACHE_PATH/00000000
+touch -d '1970-01-01' $DEBUGINFOD_CACHE_PATH/00000000 # old enough to guarantee nukage
+
 # Trigger a cache clean and run the tests again. The clients should be unable to
 # find the target.
 echo 0 > $DEBUGINFOD_CACHE_PATH/cache_clean_interval_s
@@ -586,6 +590,11 @@ if [ ! -f $DEBUGINFOD_CACHE_PATH/malformed0 ] \
   exit 1
 fi
 
+if [ -d $DEBUGINFOD_CACHE_PATH/00000000 ]; then
+    echo "failed to rmdir old cache dir"
+    exit 1
+fi
+
 # Test debuginfod without a path list; reuse $PORT1
 env LD_LIBRARY_PATH=$ldpath ${abs_builddir}/../debuginfod/debuginfod $VERBOSE -F -U -d :memory: -p $PORT1 -L -F &
 PID3=$!