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>

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index ad9dbc0a8a74dd06e6f6d28f431a4271ff75abb1..9af641ec0e13c1e2553dec64c655a1a3735dda23 100644 (file)
--- 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 7fdc6c9f30ec923d5e5bdd5f1a10d04e0f64937e..0170500faaa964f48060da46f39765d0414fa7d6 100644 (file)
--- 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 eac006d092edeaf32c67b9f430982bc5b19e5d43..0d2c5edd5e399df4cb153e61df844de2ef0a8ca2 100644 (file)
--- 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 11e849836a1011a8dd645aca122bdbb5df28fa57..3b9a5a6e80fc16f0bc6ed0c303fded935822ba11 100755 (executable)
--- 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=$!