bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: test-fdutimensat racy?


From: Bernhard Voelker
Subject: Re: test-fdutimensat racy?
Date: Tue, 21 May 2013 15:27:54 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5

On 05/21/2013 03:08 PM, Eric Blake wrote:
> On 05/21/2013 06:52 AM, Bernhard Voelker wrote:
>> +  if (-1 == fd)
>> +    {
>> +      ASSERT ((fd = creat (BASE "naptmp", 0600)) != -1); /* Never closed.  
>> */
>> +      ASSERT (unlink (BASE "naptmp") == 0);
> 
> unlink() of an open fd is not guaranteed to succeed, and indeed fails on
> mingw; you can also provoke the failure in some NFS setups.  You'd need
> to add some cleanup (maybe an atexit hook will work) that closes and
> only then removes the witness file.
> 

Good catch, thanks.
Here's a new version of the patch.

Have a nice day,
Berny


>From 503f61f658b5021225b4b1d0b749675c96de6419 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <address@hidden>
Date: Tue, 21 May 2013 15:24:54 +0200
Subject: [PATCH] tests/nap.h: avoid race

The recent change in nap.h (5191133e) decreased the probability of lost
races to about a third, however such problems could still be observed
in virtual machines and openSUSE's OBS.

Instead of calulating the nap() time once and using it (together with
a small correction multiplier), avoid the race alltogether by verifying
on a reference file whether a timestamp difference has happened.

* tests/nap.h (nap_works): Change return value to bool.  Change passing
the old file's status by value instead of by reference as this function
does no longer update that timestamp; rename the function argument from
st to old_st.  Remove the local variables cdiff and mdiff because that
function now returns true/false instead of the precise delay.
(guess_delay): Remove function.
(clear_tmp_file): Add new function to unlink the witness file.
(nap): Instead of re-using the delay which has been calulated during the
first call, avoid the race by actually verifying that a timestamp
difference can be observed on the current file system.  Use an adaptive
approach for the delay to minimize execution time.  Assert that the
maximum delay is <= 2 seconds.  Use atexit to call clear_tmp_file
when the process terminates.
---
 tests/nap.h | 107 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 46 insertions(+), 61 deletions(-)

diff --git a/tests/nap.h b/tests/nap.h
index d8dbad2..2102f98 100644
--- a/tests/nap.h
+++ b/tests/nap.h
@@ -20,6 +20,7 @@
 # define GLTEST_NAP_H

 # include <limits.h>
+#include <stdbool.h>

 /* Return A - B, in ns.
    Return 0 if the true result would be negative.
@@ -53,86 +54,70 @@ get_stat (int fd, struct stat *st, int do_write)
 }

 /* Given a file whose descriptor is FD, see whether delaying by DELAY
-   nanoseconds causes a change in a file's time stamp.  *ST is the
-   file's status, recently gotten.  Update *ST to reflect the latest
-   status gotten.  If successful, return the needed delay, in
-   nanoseconds as determined by the observed time stamps; this may be
-   greater than DELAY if we crossed a quantization boundary.  If
-   unsuccessful, return 0.  */
-static int
-nap_works (int fd, int delay, struct stat *st)
+   nanoseconds causes a change in a file's time stamp.  OLD_ST is the
+   file's status, recently gotten.  */
+static bool
+nap_works (int fd, int delay, struct stat old_st)
 {
-  struct stat old_st = *st;
+  struct stat st;
   struct timespec delay_spec;
-  int cdiff, mdiff;
   delay_spec.tv_sec = delay / 1000000000;
   delay_spec.tv_nsec = delay % 1000000000;
   ASSERT (nanosleep (&delay_spec, 0) == 0);
-  get_stat (fd, st, 1);
+  get_stat (fd, &st, 1);

   /* Return the greater of the ctime and the mtime differences, or
      zero if it cannot be determined, or INT_MAX if either overflows.  */
-  cdiff = diff_timespec (get_stat_ctime (st), get_stat_ctime (&old_st));
-  if (cdiff != 0)
-    {
-      mdiff = diff_timespec (get_stat_mtime (st), get_stat_mtime (&old_st));
-      if (mdiff != 0)
-        return cdiff < mdiff ? mdiff : cdiff;
-    }
-  return 0;
+  if (   diff_timespec (get_stat_ctime (&st), get_stat_ctime (&old_st))
+      && diff_timespec (get_stat_mtime (&st), get_stat_mtime (&old_st)))
+    return true;
+
+  return false;
 }

-static int
-guess_delay (void)
+#define TEMPFILE BASE "nap.tmp"
+
+static void
+clear_temp_file (void)
 {
-  /* Try a 1-ns sleep first, for speed.  If that doesn't work, try 100
-     ns, 1 microsecond, 1 ms, etc.  xfs has a quantization of about 10
-     milliseconds, even though it has a granularity of 1 nanosecond,
-     and NTFS has a default quantization of 15.25 milliseconds, even
-     though it has a granularity of 100 nanoseconds, so 15.25 ms is a
-     good quantization to try.  If that doesn't work, try 1 second.
-     The worst case is 2 seconds, needed for FAT.  */
-  static int const delaytab[] = {1, 1000, 1000000, 15250000, 1000000000 };
-  int fd = creat (BASE "tmp", 0600);
-  int i;
-  int delay = 2000000000;
-  struct stat st;
-  ASSERT (0 <= fd);
-  get_stat (fd, &st, 0);
-  for (i = 0; i < sizeof delaytab / sizeof delaytab[0]; i++)
-    {
-      int d = nap_works (fd, delaytab[i], &st);
-      if (d != 0)
-        {
-          delay = d;
-          break;
-        }
-    }
-  ASSERT (close (fd) == 0);
-  ASSERT (unlink (BASE "tmp") == 0);
-  return delay;
+  unlink (TEMPFILE);
 }

 /* Sleep long enough to notice a timestamp difference on the file
-   system in the current directory.  Assumes that BASE is defined,
-   and requires that the test module depends on nanosleep.  */
+   system in the current directory.  Use an adaptive approach, trying
+   to find the smallest delay which works on the current file system
+   to make the timestamp difference appear.  Assert a maximum delay of
+   2 seconds.  Assumes that BASE is defined, and requires that the test
+   module depends on nanosleep.  */
 static void
 nap (void)
 {
-  static struct timespec delay;
-  if (!delay.tv_sec && !delay.tv_nsec)
+  static int fd = -1;
+  struct stat old_st;
+  static int delay = 1;
+
+  if (-1 == fd)
+    {
+      ASSERT ((fd = creat (TEMPFILE, 0600)) != -1); /* Never closed.  */
+      atexit (clear_temp_file);
+      get_stat (fd, &old_st, 0);
+    }
+  else
     {
-      int d = guess_delay ();
-
-      /* Multiply by 1.125 (rounding up), to avoid problems if the
-         file system's clock is a bit slower than nanosleep's.
-         Ceiling it at INT_MAX, though.  */
-      int delta = (d >> 3) + ((d & 7) != 0);
-      d = delta < INT_MAX - d ? d + delta : INT_MAX;
-      delay.tv_sec = d / 1000000000;
-      delay.tv_nsec = d % 1000000000;
+      ASSERT (0 <= fd);
+      get_stat (fd, &old_st, 1);
     }
-  ASSERT (nanosleep (&delay, 0) == 0);
+
+  if (1 < delay)
+    delay = delay / 2;  /* Try half of the previous delay.  */
+  ASSERT (0 < delay);
+
+  for ( ; delay <= 2000000000; delay = delay * 2)
+    if (nap_works (fd, delay, old_st))
+      return;
+
+  /* Bummer: even the highest nap delay didn't work. */
+  ASSERT (0);
 }

 #endif /* GLTEST_NAP_H */
-- 
1.8.2.2






reply via email to

[Prev in Thread] Current Thread [Next in Thread]