[PATCH 0/5] Race condition in /etc/resolv.conf reloading (bug 25420)

[PATCH 0/5] Race condition in /etc/resolv.conf reloading (bug 25420)

[Florian Weimer]

I've split this up for easier review.  The actual changes are quite
small, and I think they clean up the code a bit.

I did not add a test case because the race is apparently quite hard to
hit.  (I still have to try the reproducer from the reporter.)

Not sure if that is still glibc 2.31 material, but it should be quite
backportable.

Thanks,
Florian

Florian Weimer (5):
  Add internal <file_change_detection.h> header file
  resolv: Use <file_change_detection.h> in __resolv_conf_get_current
  resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]
  resolv: Enhance __resolv_conf_load to capture file change data
  resolv: Fix ABA race in /etc/resolv.conf change detection [BZ #25420]

 include/file_change_detection.h | 140 ++++++++++++++++++++++
 io/Makefile                     |   2 +-
 io/tst-file_change_detection.c  | 206 ++++++++++++++++++++++++++++++++
 resolv/res_init.c               |  22 +++-
 resolv/resolv_conf.c            |  60 ++++------
 resolv/resolv_conf.h            |  10 +-
 6 files changed, 392 insertions(+), 48 deletions(-)
 create mode 100644 include/file_change_detection.h
 create mode 100644 io/tst-file_change_detection.c

-- 
2.24.1

[PATCH 1/5] Add internal <file_change_detection.h> header file

[Florian Weimer]

The code started out with bits form resolv/resolv_conf.c, but it
was enhanced to deal with directories and FIFOs in a more predictable
manner.  A test case is included as well.

This will be used to implement the /etc/resolv.conf change detection.

This currently lives in a header file only.  Once there are multiple
users, the implementations should be moved into C files.
---
 include/file_change_detection.h | 140 ++++++++++++++++++++++
 io/Makefile                     |   2 +-
 io/tst-file_change_detection.c  | 206 ++++++++++++++++++++++++++++++++
 3 files changed, 347 insertions(+), 1 deletion(-)
 create mode 100644 include/file_change_detection.h
 create mode 100644 io/tst-file_change_detection.c

diff --git a/include/file_change_detection.h b/include/file_change_detection.h
new file mode 100644
index 0000000000..aaed0a9b6d
--- /dev/null
+++ b/include/file_change_detection.h
@@ -0,0 +1,140 @@
+/* Detecting file changes using modification times.
+   Copyright (C) 2017-2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+/* Items for identifying a particular file version.  Excerpt from
+   struct stat64.  */
+struct file_change_detection
+{
+  /* Special values: 0 if file does not exist.  -1 to force mismatch
+     with the next comparison.  */
+  off64_t size;
+
+  ino64_t ino;
+  struct timespec mtime;
+  struct timespec ctime;
+};
+
+/* Returns true if *LEFT and *RIGHT describe the same version of the
+   same file.  */
+static bool __attribute__ ((unused))
+file_is_unchanged (const struct file_change_detection *left,
+                   const struct file_change_detection *right)
+{
+  if (left->size < 0 || right->size < 0)
+    /* Negative sizes are used as markers and never match.  */
+    return false;
+  else if (left->size == 0 && right->size == 0)
+    /* Both files are empty or do not exist, so they have the same
+       content, no matter what the other fields indicate.  */
+    return true;
+  else
+    return left->size == right->size
+      && left->ino == right->ino
+      && left->mtime.tv_sec == right->mtime.tv_sec
+      && left->mtime.tv_nsec == right->mtime.tv_nsec
+      && left->ctime.tv_sec == right->ctime.tv_sec
+      && left->ctime.tv_nsec == right->ctime.tv_nsec;
+}
+
+/* Extract file change information to *FILE from the stat buffer
+   *ST.  */
+static void __attribute__ ((unused))
+file_change_detection_for_stat (struct file_change_detection *file,
+                                const struct stat64 *st)
+{
+  if (S_ISDIR (st->st_mode))
+    /* Treat as empty file.  */
+    file->size = 0;
+  else if (!S_ISREG (st->st_mode))
+    /* Non-regular files cannot be cached.  */
+    file->size = -1;
+  else
+    {
+      file->size = st->st_size;
+      file->ino = st->st_ino;
+      file->mtime = st->st_mtim;
+      file->ctime = st->st_ctim;
+    }
+}
+
+/* Writes file change information for PATH to *FILE.  Returns true on
+   success.  For benign errors, *FILE is cleared, and true is
+   returned.  For errors indicating resource outages and the like,
+   false is returned.  */
+static bool __attribute__ ((unused))
+file_change_detection_for_path (struct file_change_detection *file,
+                                const char *path)
+{
+  struct stat64 st;
+  if (stat64 (path, &st) != 0)
+    switch (errno)
+      {
+      case EACCES:
+      case EISDIR:
+      case ELOOP:
+      case ENOENT:
+      case ENOTDIR:
+      case EPERM:
+        /* Ignore errors due to file system contents.  Instead, treat
+           the file as empty.  */
+        file->size = 0;
+        return true;
+      default:
+        /* Other errors are fatal.  */
+        return false;
+      }
+  else /* stat64 was successfull.  */
+    {
+      file_change_detection_for_stat (file, &st);
+      return true;
+    }
+}
+
+/* Writes file change information for the stream FP to *FILE.  Returns
+   ture on success, false on failure.  If FP is NULL, treat the file
+   as non-existing.  */
+static bool __attribute__ ((unused))
+file_change_detection_for_fp (struct file_change_detection *file,
+                              FILE *fp)
+{
+  if (fp == NULL)
+    {
+      /* The file does not exist.  */
+      file->size = 0;
+      return true;
+    }
+  else
+    {
+      struct stat64 st;
+      if (fstat64 (__fileno (fp), &st) != 0)
+        /* If we already have a file descriptor, all errors are fatal.  */
+        return false;
+      else
+        {
+          file_change_detection_for_stat (file, &st);
+          return true;
+        }
+    }
+}
diff --git a/io/Makefile b/io/Makefile
index d9a1da4566..437a7732f0 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -74,7 +74,7 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
 		   tst-posix_fallocate tst-posix_fallocate64 \
 		   tst-fts tst-fts-lfs tst-open-tmpfile \
 		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
-		   tst-ftw-lnk
+		   tst-ftw-lnk tst-file_change_detection
 
 # Likewise for statx, but we do not need static linking here.
 tests-internal += tst-statx
diff --git a/io/tst-file_change_detection.c b/io/tst-file_change_detection.c
new file mode 100644
index 0000000000..035dd39c4d
--- /dev/null
+++ b/io/tst-file_change_detection.c
@@ -0,0 +1,206 @@
+/* Test for <file_change_detection.c>.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+/* The header uses the internal __fileno symbol, which is not
+   available outside of libc (even to internal tests).  */
+#define __fileno(fp) fileno (fp)
+
+#include <file_change_detection.h>
+
+#include <array_length.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xstdio.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+
+static void
+all_same (struct file_change_detection *array, size_t length)
+{
+  for (size_t i = 0; i < length; ++i)
+    for (size_t j = 0; j < length; ++j)
+      {
+        if (test_verbose > 0)
+          printf ("info: comparing %zu and %zu\n", i, j);
+        TEST_VERIFY (file_is_unchanged (array + i, array + j));
+      }
+}
+
+static void
+all_different (struct file_change_detection *array, size_t length)
+{
+  for (size_t i = 0; i < length; ++i)
+    for (size_t j = 0; j < length; ++j)
+      {
+        if (i == j)
+          continue;
+        if (test_verbose > 0)
+          printf ("info: comparing %zu and %zu\n", i, j);
+        TEST_VERIFY (!file_is_unchanged (array + i, array + j));
+      }
+}
+
+static int
+do_test (void)
+{
+  /* Use a temporary directory with various paths.  */
+  char *tempdir = support_create_temp_directory ("tst-file_change_detection-");
+
+  char *path_dangling = xasprintf ("%s/dangling", tempdir);
+  char *path_does_not_exist = xasprintf ("%s/does-not-exist", tempdir);
+  char *path_empty1 = xasprintf ("%s/empty1", tempdir);
+  char *path_empty2 = xasprintf ("%s/empty2", tempdir);
+  char *path_fifo = xasprintf ("%s/fifo", tempdir);
+  char *path_file1 = xasprintf ("%s/file1", tempdir);
+  char *path_file2 = xasprintf ("%s/file2", tempdir);
+  char *path_loop = xasprintf ("%s/loop", tempdir);
+  char *path_to_empty1 = xasprintf ("%s/to-empty1", tempdir);
+  char *path_to_file1 = xasprintf ("%s/to-file1", tempdir);
+
+  add_temp_file (path_dangling);
+  add_temp_file (path_empty1);
+  add_temp_file (path_empty2);
+  add_temp_file (path_fifo);
+  add_temp_file (path_file1);
+  add_temp_file (path_file2);
+  add_temp_file (path_loop);
+  add_temp_file (path_to_empty1);
+  add_temp_file (path_to_file1);
+
+  xsymlink ("target-does-not-exist", path_dangling);
+  support_write_file_string (path_empty1, "");
+  support_write_file_string (path_empty2, "");
+  TEST_COMPARE (mknod (path_fifo, 0777 | S_IFIFO, 0), 0);
+  support_write_file_string (path_file1, "line\n");
+  support_write_file_string (path_file2, "line\n");
+  xsymlink ("loop", path_loop);
+  xsymlink ("empty1", path_to_empty1);
+  xsymlink ("file1", path_to_file1);
+
+  FILE *fp_file1 = xfopen (path_file1, "r");
+  FILE *fp_file2 = xfopen (path_file2, "r");
+  FILE *fp_empty1 = xfopen (path_empty1, "r");
+  FILE *fp_empty2 = xfopen (path_empty2, "r");
+
+  /* Test for the same (empty) files.  */
+  {
+    struct file_change_detection fcd[10];
+    int i = 0;
+    /* Two empty files always have the same contents.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty1));
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty2));
+    /* So does a missing file (which is treated as empty).  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++],
+                                                 path_does_not_exist));
+    /* And a symbolic link loop.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_loop));
+    /* And a dangling symbolic link.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_dangling));
+    /* And a directory.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], tempdir));
+    /* And a symbolic link to an empty file.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_to_empty1));
+    /* Likewise for access the file via a FILE *.  */
+    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_empty1));
+    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_empty2));
+    /* And a NULL FILE * (missing file).  */
+    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], NULL));
+    TEST_COMPARE (i, array_length (fcd));
+
+    all_same (fcd, array_length (fcd));
+  }
+
+  /* Symbolic links are resolved.  */
+  {
+    struct file_change_detection fcd[3];
+    int i = 0;
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file1));
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_to_file1));
+    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_file1));
+    TEST_COMPARE (i, array_length (fcd));
+    all_same (fcd, array_length (fcd));
+  }
+
+  /* Test for different files.  */
+  {
+    struct file_change_detection fcd[5];
+    int i = 0;
+    /* The other files are not empty.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty1));
+    /* These two files have the same contents, but have different file
+       identity.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file1));
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file2));
+    /* FIFOs are always different, even with themselves.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_fifo));
+    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_fifo));
+    TEST_COMPARE (i, array_length (fcd));
+    all_different (fcd, array_length (fcd));
+
+    /* Replacing the file with its symbolic link does not make a
+       difference.  */
+    TEST_VERIFY (file_change_detection_for_path (&fcd[1], path_to_file1));
+    all_different (fcd, array_length (fcd));
+  }
+
+  /* Wait for a file change.  Depending on file system time stamp
+     resolution, this subtest blocks for a while.  */
+  for (int use_stdio = 0; use_stdio < 2; ++use_stdio)
+    {
+      struct file_change_detection initial;
+      TEST_VERIFY (file_change_detection_for_path (&initial, path_file1));
+      while (true)
+        {
+          support_write_file_string (path_file1, "line\n");
+          struct file_change_detection current;
+          if (use_stdio)
+            TEST_VERIFY (file_change_detection_for_fp (&current, fp_file1));
+          else
+            TEST_VERIFY (file_change_detection_for_path (&current, path_file1));
+          if (!file_is_unchanged (&initial, &current))
+            break;
+          /* Wait for a bit to reduce system load.  */
+          usleep (100 * 1000);
+        }
+    }
+
+  fclose (fp_empty1);
+  fclose (fp_empty2);
+  fclose (fp_file1);
+  fclose (fp_file2);
+
+  free (path_dangling);
+  free (path_does_not_exist);
+  free (path_empty1);
+  free (path_empty2);
+  free (path_fifo);
+  free (path_file1);
+  free (path_file2);
+  free (path_loop);
+  free (path_to_empty1);
+  free (path_to_file1);
+
+  free (tempdir);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.24.1

Re: [PATCH 1/5] Add internal <file_change_detection.h> header file

[Adhemerval Zanella]

On 21/01/2020 15:41, Florian Weimer wrote:
> The code started out with bits form resolv/resolv_conf.c, but it
> was enhanced to deal with directories and FIFOs in a more predictable
> manner.  A test case is included as well.
> 
> This will be used to implement the /etc/resolv.conf change detection.
> 
> This currently lives in a header file only.  Once there are multiple
> users, the implementations should be moved into C files.

LGTM, with some nits below.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  include/file_change_detection.h | 140 ++++++++++++++++++++++
>  io/Makefile                     |   2 +-
>  io/tst-file_change_detection.c  | 206 ++++++++++++++++++++++++++++++++
>  3 files changed, 347 insertions(+), 1 deletion(-)
>  create mode 100644 include/file_change_detection.h
>  create mode 100644 io/tst-file_change_detection.c
> 
> diff --git a/include/file_change_detection.h b/include/file_change_detection.h
> new file mode 100644
> index 0000000000..aaed0a9b6d
> --- /dev/null
> +++ b/include/file_change_detection.h
> @@ -0,0 +1,140 @@
> +/* Detecting file changes using modification times.
> +   Copyright (C) 2017-2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stddef.h>
> +#include <stdio.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +/* Items for identifying a particular file version.  Excerpt from
> +   struct stat64.  */
> +struct file_change_detection
> +{
> +  /* Special values: 0 if file does not exist.  -1 to force mismatch
> +     with the next comparison.  */
> +  off64_t size;
> +
> +  ino64_t ino;
> +  struct timespec mtime;
> +  struct timespec ctime;
> +};
> +

Ok.

> +/* Returns true if *LEFT and *RIGHT describe the same version of the
> +   same file.  */
> +static bool __attribute__ ((unused))
> +file_is_unchanged (const struct file_change_detection *left,
> +                   const struct file_change_detection *right)
> +{
> +  if (left->size < 0 || right->size < 0)
> +    /* Negative sizes are used as markers and never match.  */
> +    return false;
> +  else if (left->size == 0 && right->size == 0)
> +    /* Both files are empty or do not exist, so they have the same
> +       content, no matter what the other fields indicate.  */
> +    return true;
> +  else
> +    return left->size == right->size
> +      && left->ino == right->ino
> +      && left->mtime.tv_sec == right->mtime.tv_sec
> +      && left->mtime.tv_nsec == right->mtime.tv_nsec
> +      && left->ctime.tv_sec == right->ctime.tv_sec
> +      && left->ctime.tv_nsec == right->ctime.tv_nsec;
> +}

Ok.

> +
> +/* Extract file change information to *FILE from the stat buffer
> +   *ST.  */
> +static void __attribute__ ((unused))
> +file_change_detection_for_stat (struct file_change_detection *file,
> +                                const struct stat64 *st)
> +{
> +  if (S_ISDIR (st->st_mode))
> +    /* Treat as empty file.  */
> +    file->size = 0;
> +  else if (!S_ISREG (st->st_mode))
> +    /* Non-regular files cannot be cached.  */
> +    file->size = -1;
> +  else
> +    {
> +      file->size = st->st_size;
> +      file->ino = st->st_ino;
> +      file->mtime = st->st_mtim;
> +      file->ctime = st->st_ctim;
> +    }
> +}

Ok.

> +
> +/* Writes file change information for PATH to *FILE.  Returns true on
> +   success.  For benign errors, *FILE is cleared, and true is
> +   returned.  For errors indicating resource outages and the like,
> +   false is returned.  */
> +static bool __attribute__ ((unused))
> +file_change_detection_for_path (struct file_change_detection *file,
> +                                const char *path)
> +{
> +  struct stat64 st;
> +  if (stat64 (path, &st) != 0)
> +    switch (errno)
> +      {
> +      case EACCES:
> +      case EISDIR:
> +      case ELOOP:
> +      case ENOENT:
> +      case ENOTDIR:
> +      case EPERM:
> +        /* Ignore errors due to file system contents.  Instead, treat
> +           the file as empty.  */
> +        file->size = 0;
> +        return true;
> +      default:
> +        /* Other errors are fatal.  */
> +        return false;
> +      }
> +  else /* stat64 was successfull.  */
> +    {
> +      file_change_detection_for_stat (file, &st);
> +      return true;
> +    }
> +}

Ok.

> +
> +/* Writes file change information for the stream FP to *FILE.  Returns
> +   ture on success, false on failure.  If FP is NULL, treat the file
> +   as non-existing.  */
> +static bool __attribute__ ((unused))
> +file_change_detection_for_fp (struct file_change_detection *file,
> +                              FILE *fp)
> +{
> +  if (fp == NULL)
> +    {
> +      /* The file does not exist.  */
> +      file->size = 0;
> +      return true;
> +    }
> +  else
> +    {
> +      struct stat64 st;
> +      if (fstat64 (__fileno (fp), &st) != 0)
> +        /* If we already have a file descriptor, all errors are fatal.  */
> +        return false;
> +      else
> +        {
> +          file_change_detection_for_stat (file, &st);
> +          return true;
> +        }
> +    }
> +}

Ok.

> diff --git a/io/Makefile b/io/Makefile
> index d9a1da4566..437a7732f0 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -74,7 +74,7 @@ tests		:= test-utime test-stat test-stat2 test-lfs tst-getcwd \
>  		   tst-posix_fallocate tst-posix_fallocate64 \
>  		   tst-fts tst-fts-lfs tst-open-tmpfile \
>  		   tst-copy_file_range tst-getcwd-abspath tst-lockf \
> -		   tst-ftw-lnk
> +		   tst-ftw-lnk tst-file_change_detection
>  
>  # Likewise for statx, but we do not need static linking here.
>  tests-internal += tst-statx

Ok.

> diff --git a/io/tst-file_change_detection.c b/io/tst-file_change_detection.c
> new file mode 100644
> index 0000000000..035dd39c4d
> --- /dev/null
> +++ b/io/tst-file_change_detection.c
> @@ -0,0 +1,206 @@
> +/* Test for <file_change_detection.c>.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +/* The header uses the internal __fileno symbol, which is not
> +   available outside of libc (even to internal tests).  */
> +#define __fileno(fp) fileno (fp)

Ok.

> +
> +#include <file_change_detection.h>
> +
> +#include <array_length.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +static void
> +all_same (struct file_change_detection *array, size_t length)
> +{
> +  for (size_t i = 0; i < length; ++i)
> +    for (size_t j = 0; j < length; ++j)
> +      {
> +        if (test_verbose > 0)
> +          printf ("info: comparing %zu and %zu\n", i, j);
> +        TEST_VERIFY (file_is_unchanged (array + i, array + j));
> +      }
> +}
> +

Wouldn't it the following be slight better?

  for (size_t i = 0; i < length - 1; ++i)
    for (size_t j = i + 1 < length, j++)
       
Or do you really to check all possible permutations?

> +static void
> +all_different (struct file_change_detection *array, size_t length)
> +{
> +  for (size_t i = 0; i < length; ++i)
> +    for (size_t j = 0; j < length; ++j)
> +      {
> +        if (i == j)
> +          continue;
> +        if (test_verbose > 0)
> +          printf ("info: comparing %zu and %zu\n", i, j);
> +        TEST_VERIFY (!file_is_unchanged (array + i, array + j));
> +      }
> +}

Same as before.

> +
> +static int
> +do_test (void)
> +{
> +  /* Use a temporary directory with various paths.  */
> +  char *tempdir = support_create_temp_directory ("tst-file_change_detection-");
> +
> +  char *path_dangling = xasprintf ("%s/dangling", tempdir);
> +  char *path_does_not_exist = xasprintf ("%s/does-not-exist", tempdir);
> +  char *path_empty1 = xasprintf ("%s/empty1", tempdir);
> +  char *path_empty2 = xasprintf ("%s/empty2", tempdir);
> +  char *path_fifo = xasprintf ("%s/fifo", tempdir);
> +  char *path_file1 = xasprintf ("%s/file1", tempdir);
> +  char *path_file2 = xasprintf ("%s/file2", tempdir);
> +  char *path_loop = xasprintf ("%s/loop", tempdir);
> +  char *path_to_empty1 = xasprintf ("%s/to-empty1", tempdir);
> +  char *path_to_file1 = xasprintf ("%s/to-file1", tempdir);
> +
> +  add_temp_file (path_dangling);
> +  add_temp_file (path_empty1);
> +  add_temp_file (path_empty2);
> +  add_temp_file (path_fifo);
> +  add_temp_file (path_file1);
> +  add_temp_file (path_file2);
> +  add_temp_file (path_loop);
> +  add_temp_file (path_to_empty1);
> +  add_temp_file (path_to_file1);
> +
> +  xsymlink ("target-does-not-exist", path_dangling);
> +  support_write_file_string (path_empty1, "");
> +  support_write_file_string (path_empty2, "");
> +  TEST_COMPARE (mknod (path_fifo, 0777 | S_IFIFO, 0), 0);
> +  support_write_file_string (path_file1, "line\n");
> +  support_write_file_string (path_file2, "line\n");
> +  xsymlink ("loop", path_loop);
> +  xsymlink ("empty1", path_to_empty1);
> +  xsymlink ("file1", path_to_file1);

Ok.

> +
> +  FILE *fp_file1 = xfopen (path_file1, "r");
> +  FILE *fp_file2 = xfopen (path_file2, "r");
> +  FILE *fp_empty1 = xfopen (path_empty1, "r");
> +  FILE *fp_empty2 = xfopen (path_empty2, "r");
> +
> +  /* Test for the same (empty) files.  */
> +  {
> +    struct file_change_detection fcd[10];
> +    int i = 0;
> +    /* Two empty files always have the same contents.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty1));
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty2));
> +    /* So does a missing file (which is treated as empty).  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++],
> +                                                 path_does_not_exist));
> +    /* And a symbolic link loop.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_loop));
> +    /* And a dangling symbolic link.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_dangling));
> +    /* And a directory.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], tempdir));
> +    /* And a symbolic link to an empty file.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_to_empty1));
> +    /* Likewise for access the file via a FILE *.  */
> +    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_empty1));
> +    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_empty2));
> +    /* And a NULL FILE * (missing file).  */
> +    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], NULL));
> +    TEST_COMPARE (i, array_length (fcd));
> +
> +    all_same (fcd, array_length (fcd));
> +  }

Ok.

> +
> +  /* Symbolic links are resolved.  */
> +  {
> +    struct file_change_detection fcd[3];
> +    int i = 0;
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file1));
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_to_file1));
> +    TEST_VERIFY (file_change_detection_for_fp (&fcd[i++], fp_file1));
> +    TEST_COMPARE (i, array_length (fcd));
> +    all_same (fcd, array_length (fcd));
> +  }

Ok.

> +
> +  /* Test for different files.  */
> +  {
> +    struct file_change_detection fcd[5];
> +    int i = 0;
> +    /* The other files are not empty.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_empty1));
> +    /* These two files have the same contents, but have different file
> +       identity.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file1));
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_file2));
> +    /* FIFOs are always different, even with themselves.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_fifo));
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[i++], path_fifo));
> +    TEST_COMPARE (i, array_length (fcd));
> +    all_different (fcd, array_length (fcd));
> +
> +    /* Replacing the file with its symbolic link does not make a
> +       difference.  */
> +    TEST_VERIFY (file_change_detection_for_path (&fcd[1], path_to_file1));
> +    all_different (fcd, array_length (fcd));
> +  }

Ok.

> +
> +  /* Wait for a file change.  Depending on file system time stamp
> +     resolution, this subtest blocks for a while.  */
> +  for (int use_stdio = 0; use_stdio < 2; ++use_stdio)
> +    {
> +      struct file_change_detection initial;
> +      TEST_VERIFY (file_change_detection_for_path (&initial, path_file1));
> +      while (true)
> +        {
> +          support_write_file_string (path_file1, "line\n");
> +          struct file_change_detection current;
> +          if (use_stdio)
> +            TEST_VERIFY (file_change_detection_for_fp (&current, fp_file1));
> +          else
> +            TEST_VERIFY (file_change_detection_for_path (&current, path_file1));
> +          if (!file_is_unchanged (&initial, &current))
> +            break;
> +          /* Wait for a bit to reduce system load.  */
> +          usleep (100 * 1000);
> +        }
> +    }

Ok, although the usleep seems excessive large (the testing will most likely
timeout prior usleep return).

> +
> +  fclose (fp_empty1);
> +  fclose (fp_empty2);
> +  fclose (fp_file1);
> +  fclose (fp_file2);
> +
> +  free (path_dangling);
> +  free (path_does_not_exist);
> +  free (path_empty1);
> +  free (path_empty2);
> +  free (path_fifo);
> +  free (path_file1);
> +  free (path_file2);
> +  free (path_loop);
> +  free (path_to_empty1);
> +  free (path_to_file1);
> +
> +  free (tempdir);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 

Ok.

Re: [PATCH 1/5] Add internal <file_change_detection.h> header file

[Florian Weimer]

* Adhemerval Zanella:

> On 21/01/2020 15:41, Florian Weimer wrote:
>> The code started out with bits form resolv/resolv_conf.c, but it
>> was enhanced to deal with directories and FIFOs in a more predictable
>> manner.  A test case is included as well.
>> 
>> This will be used to implement the /etc/resolv.conf change detection.
>> 
>> This currently lives in a header file only.  Once there are multiple
>> users, the implementations should be moved into C files.
>
> LGTM, with some nits below.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

Thanks.
>> +static void
>> +all_same (struct file_change_detection *array, size_t length)
>> +{
>> +  for (size_t i = 0; i < length; ++i)
>> +    for (size_t j = 0; j < length; ++j)
>> +      {
>> +        if (test_verbose > 0)
>> +          printf ("info: comparing %zu and %zu\n", i, j);
>> +        TEST_VERIFY (file_is_unchanged (array + i, array + j));
>> +      }
>> +}
>> +
>
> Wouldn't it the following be slight better?
>
>   for (size_t i = 0; i < length - 1; ++i)
>     for (size_t j = i + 1 < length, j++)
>        
> Or do you really to check all possible permutations?

Iterating over all pairs checks that the operation is commutative.

>> +  /* Wait for a file change.  Depending on file system time stamp
>> +     resolution, this subtest blocks for a while.  */
>> +  for (int use_stdio = 0; use_stdio < 2; ++use_stdio)
>> +    {
>> +      struct file_change_detection initial;
>> +      TEST_VERIFY (file_change_detection_for_path (&initial, path_file1));
>> +      while (true)
>> +        {
>> +          support_write_file_string (path_file1, "line\n");
>> +          struct file_change_detection current;
>> +          if (use_stdio)
>> +            TEST_VERIFY (file_change_detection_for_fp (&current, fp_file1));
>> +          else
>> +            TEST_VERIFY (file_change_detection_for_path (&current, path_file1));
>> +          if (!file_is_unchanged (&initial, &current))
>> +            break;
>> +          /* Wait for a bit to reduce system load.  */
>> +          usleep (100 * 1000);
>> +        }
>> +    }
>
> Ok, although the usleep seems excessive large (the testing will most likely
> timeout prior usleep return).

Hmm, I thought that this would wait 100 milliseconds?  So the loop
should exit in a second or two with low-resolution timestamps in the
file system.

Re: [PATCH 1/5] Add internal <file_change_detection.h> header file

[Adhemerval Zanella]

On 10/02/2020 16:57, Florian Weimer wrote:
> 
>>> +  /* Wait for a file change.  Depending on file system time stamp
>>> +     resolution, this subtest blocks for a while.  */
>>> +  for (int use_stdio = 0; use_stdio < 2; ++use_stdio)
>>> +    {
>>> +      struct file_change_detection initial;
>>> +      TEST_VERIFY (file_change_detection_for_path (&initial, path_file1));
>>> +      while (true)
>>> +        {
>>> +          support_write_file_string (path_file1, "line\n");
>>> +          struct file_change_detection current;
>>> +          if (use_stdio)
>>> +            TEST_VERIFY (file_change_detection_for_fp (&current, fp_file1));
>>> +          else
>>> +            TEST_VERIFY (file_change_detection_for_path (&current, path_file1));
>>> +          if (!file_is_unchanged (&initial, &current))
>>> +            break;
>>> +          /* Wait for a bit to reduce system load.  */
>>> +          usleep (100 * 1000);
>>> +        }
>>> +    }
>>
>> Ok, although the usleep seems excessive large (the testing will most likely
>> timeout prior usleep return).
> 
> Hmm, I thought that this would wait 100 milliseconds?  So the loop
> should exit in a second or two with low-resolution timestamps in the
> file system.
> 

Right, it should be ok then.

[PATCH 4/5] resolv: Enhance __resolv_conf_load to capture file change data

[Florian Weimer]

The data is captured after reading the file.  This allows callers
to check the change data against an earlier measurement.
---
 resolv/res_init.c    | 14 +++++++++++---
 resolv/resolv_conf.c |  2 +-
 resolv/resolv_conf.h | 10 +++++++---
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/resolv/res_init.c b/resolv/res_init.c
index 09345718cd..98d84f264d 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -103,6 +103,7 @@
 #include <inet/net-internal.h>
 #include <errno.h>
 #include <resolv_conf.h>
+#include <file_change_detection.h>
 
 static uint32_t net_mask (struct in_addr);
 
@@ -549,7 +550,8 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
 }
 
 struct resolv_conf *
-__resolv_conf_load (struct __res_state *preinit)
+__resolv_conf_load (struct __res_state *preinit,
+                    struct file_change_detection *change)
 {
   /* Ensure that /etc/hosts.conf has been loaded (once).  */
   _res_hconf_init ();
@@ -577,7 +579,13 @@ __resolv_conf_load (struct __res_state *preinit)
   resolv_conf_parser_init (&parser, preinit);
 
   struct resolv_conf *conf = NULL;
-  if (res_vinit_1 (fp, &parser))
+  bool ok = res_vinit_1 (fp, &parser);
+  if (ok && change != NULL)
+    /* Update the file change information if the configuration was
+       loaded successfully.  */
+    ok = file_change_detection_for_fp (change, fp);
+
+  if (ok)
     {
       parser.template.nameserver_list
         = nameserver_list_begin (&parser.nameserver_list);
@@ -615,7 +623,7 @@ __res_vinit (res_state statp, int preinit)
   if (preinit && has_preinit_values (statp))
     /* For the preinit case, we cannot use the cached configuration
        because some settings could be different.  */
-    conf = __resolv_conf_load (statp);
+    conf = __resolv_conf_load (statp, NULL);
   else
     conf = __resolv_conf_get_current ();
   if (conf == NULL)
diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
index d954ba9a5a..bdd2ebb909 100644
--- a/resolv/resolv_conf.c
+++ b/resolv/resolv_conf.c
@@ -136,7 +136,7 @@ __resolv_conf_get_current (void)
     {
       /* Parse configuration while holding the lock.  This avoids
          duplicate work.  */
-      conf = __resolv_conf_load (NULL);
+      conf = __resolv_conf_load (NULL, NULL);
       if (conf != NULL)
         {
           if (global_copy->conf_current != NULL)
diff --git a/resolv/resolv_conf.h b/resolv/resolv_conf.h
index 01cbff9111..101e14bfe5 100644
--- a/resolv/resolv_conf.h
+++ b/resolv/resolv_conf.h
@@ -63,12 +63,16 @@ struct resolv_conf
    and the struct resolv_context facility.  */
 
 struct __res_state;
+struct file_change_detection;
 
 /* Read /etc/resolv.conf and return a configuration object, or NULL if
    /etc/resolv.conf cannot be read due to memory allocation errors.
-   If PREINIT is not NULL, some configuration values are taken from the
-   struct __res_state object.  */
-struct resolv_conf *__resolv_conf_load (struct __res_state *preinit)
+   If PREINIT is not NULL, some configuration values are taken from
+   the struct __res_state object.  If CHANGE is not null, file change
+   detection data is written to *CHANGE, based on the state of the
+   file after reading it.  */
+struct resolv_conf *__resolv_conf_load (struct __res_state *preinit,
+                                        struct file_change_detection *change)
   attribute_hidden __attribute__ ((warn_unused_result));
 
 /* Return a configuration object for the current /etc/resolv.conf
-- 
2.24.1

Re: [PATCH 4/5] resolv: Enhance __resolv_conf_load to capture file change data

[Adhemerval Zanella]

On 21/01/2020 15:42, Florian Weimer wrote:
> The data is captured after reading the file.  This allows callers
> to check the change data against an earlier measurement.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  resolv/res_init.c    | 14 +++++++++++---
>  resolv/resolv_conf.c |  2 +-
>  resolv/resolv_conf.h | 10 +++++++---
>  3 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index 09345718cd..98d84f264d 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -103,6 +103,7 @@
>  #include <inet/net-internal.h>
>  #include <errno.h>
>  #include <resolv_conf.h>
> +#include <file_change_detection.h>
>  
>  static uint32_t net_mask (struct in_addr);
>  

Ok.

> @@ -549,7 +550,8 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
>  }
>  
>  struct resolv_conf *
> -__resolv_conf_load (struct __res_state *preinit)
> +__resolv_conf_load (struct __res_state *preinit,
> +                    struct file_change_detection *change)
>  {
>    /* Ensure that /etc/hosts.conf has been loaded (once).  */
>    _res_hconf_init ();

Ok.

> @@ -577,7 +579,13 @@ __resolv_conf_load (struct __res_state *preinit)
>    resolv_conf_parser_init (&parser, preinit);
>  
>    struct resolv_conf *conf = NULL;
> -  if (res_vinit_1 (fp, &parser))
> +  bool ok = res_vinit_1 (fp, &parser);
> +  if (ok && change != NULL)
> +    /* Update the file change information if the configuration was
> +       loaded successfully.  */
> +    ok = file_change_detection_for_fp (change, fp);
> +
> +  if (ok)
>      {
>        parser.template.nameserver_list
>          = nameserver_list_begin (&parser.nameserver_list);

Ok.

> @@ -615,7 +623,7 @@ __res_vinit (res_state statp, int preinit)
>    if (preinit && has_preinit_values (statp))
>      /* For the preinit case, we cannot use the cached configuration
>         because some settings could be different.  */
> -    conf = __resolv_conf_load (statp);
> +    conf = __resolv_conf_load (statp, NULL);
>    else
>      conf = __resolv_conf_get_current ();
>    if (conf == NULL)

Ok.

> diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
> index d954ba9a5a..bdd2ebb909 100644
> --- a/resolv/resolv_conf.c
> +++ b/resolv/resolv_conf.c
> @@ -136,7 +136,7 @@ __resolv_conf_get_current (void)
>      {
>        /* Parse configuration while holding the lock.  This avoids
>           duplicate work.  */
> -      conf = __resolv_conf_load (NULL);
> +      conf = __resolv_conf_load (NULL, NULL);
>        if (conf != NULL)
>          {
>            if (global_copy->conf_current != NULL)

Ok.

> diff --git a/resolv/resolv_conf.h b/resolv/resolv_conf.h
> index 01cbff9111..101e14bfe5 100644
> --- a/resolv/resolv_conf.h
> +++ b/resolv/resolv_conf.h
> @@ -63,12 +63,16 @@ struct resolv_conf
>     and the struct resolv_context facility.  */
>  
>  struct __res_state;
> +struct file_change_detection;
>  
>  /* Read /etc/resolv.conf and return a configuration object, or NULL if
>     /etc/resolv.conf cannot be read due to memory allocation errors.
> -   If PREINIT is not NULL, some configuration values are taken from the
> -   struct __res_state object.  */
> -struct resolv_conf *__resolv_conf_load (struct __res_state *preinit)
> +   If PREINIT is not NULL, some configuration values are taken from
> +   the struct __res_state object.  If CHANGE is not null, file change
> +   detection data is written to *CHANGE, based on the state of the
> +   file after reading it.  */
> +struct resolv_conf *__resolv_conf_load (struct __res_state *preinit,
> +                                        struct file_change_detection *change)
>    attribute_hidden __attribute__ ((warn_unused_result));
>  
>  /* Return a configuration object for the current /etc/resolv.conf
> 

Ok.

[PATCH 3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]

[Florian Weimer]

res_vinit_1 did not close the stream on errors, only on success.
This change moves closing the stream to __resolv_conf_load, for both
the success and error cases.

Fixes commit 89f187a40fc0ad4e22838526bfe34d73f758b776 ("resolv: Use
getline for configuration file reading in res_vinit_1") and commit
3f853f22c87f0b671c0366eb290919719fa56c0e ("resolv: Lift domain search
list limits [BZ #19569] [BZ #21475]"), where memory allocation was
introduced into res_vinit_1.
---
 resolv/res_init.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/resolv/res_init.c b/resolv/res_init.c
index 95dce098aa..09345718cd 100644
--- a/resolv/res_init.c
+++ b/resolv/res_init.c
@@ -508,7 +508,6 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
               continue;
             }
         }
-      fclose (fp);
     }
   if (__glibc_unlikely (nameserver_list_size (&parser->nameserver_list) == 0))
     {
@@ -593,6 +592,13 @@ __resolv_conf_load (struct __res_state *preinit)
     }
   resolv_conf_parser_free (&parser);
 
+  if (fp != NULL)
+    {
+      int saved_errno = errno;
+      fclose (fp);
+      __set_errno (saved_errno);
+    }
+
   return conf;
 }
 
-- 
2.24.1

Re: [PATCH 3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]

[Adhemerval Zanella]

On 21/01/2020 15:41, Florian Weimer wrote:
> res_vinit_1 did not close the stream on errors, only on success.
> This change moves closing the stream to __resolv_conf_load, for both
> the success and error cases.
> 
> Fixes commit 89f187a40fc0ad4e22838526bfe34d73f758b776 ("resolv: Use
> getline for configuration file reading in res_vinit_1") and commit
> 3f853f22c87f0b671c0366eb290919719fa56c0e ("resolv: Lift domain search
> list limits [BZ #19569] [BZ #21475]"), where memory allocation was
> introduced into res_vinit_1.
> ---
>  resolv/res_init.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/resolv/res_init.c b/resolv/res_init.c
> index 95dce098aa..09345718cd 100644
> --- a/resolv/res_init.c
> +++ b/resolv/res_init.c
> @@ -508,7 +508,6 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
>                continue;
>              }
>          }
> -      fclose (fp);
>      }
>    if (__glibc_unlikely (nameserver_list_size (&parser->nameserver_list) == 0))
>      {
> @@ -593,6 +592,13 @@ __resolv_conf_load (struct __res_state *preinit)
>      }
>    resolv_conf_parser_free (&parser);
>  
> +  if (fp != NULL)
> +    {
> +      int saved_errno = errno;
> +      fclose (fp);
> +      __set_errno (saved_errno);
> +    }
> +
>    return conf;
>  }

Why not close the FILE on __resolv_conf_load? It make the FILE object 
cleanup as close as its creation, which usually improves readability.

Re: [PATCH 3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]

[Florian Weimer]

* Adhemerval Zanella:

> On 21/01/2020 15:41, Florian Weimer wrote:
>> res_vinit_1 did not close the stream on errors, only on success.
>> This change moves closing the stream to __resolv_conf_load, for both
>> the success and error cases.
>> 
>> Fixes commit 89f187a40fc0ad4e22838526bfe34d73f758b776 ("resolv: Use
>> getline for configuration file reading in res_vinit_1") and commit
>> 3f853f22c87f0b671c0366eb290919719fa56c0e ("resolv: Lift domain search
>> list limits [BZ #19569] [BZ #21475]"), where memory allocation was
>> introduced into res_vinit_1.
>> ---
>>  resolv/res_init.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/resolv/res_init.c b/resolv/res_init.c
>> index 95dce098aa..09345718cd 100644
>> --- a/resolv/res_init.c
>> +++ b/resolv/res_init.c
>> @@ -508,7 +508,6 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
>>                continue;
>>              }
>>          }
>> -      fclose (fp);
>>      }
>>    if (__glibc_unlikely (nameserver_list_size (&parser->nameserver_list) == 0))
>>      {
>> @@ -593,6 +592,13 @@ __resolv_conf_load (struct __res_state *preinit)
>>      }
>>    resolv_conf_parser_free (&parser);
>>  
>> +  if (fp != NULL)
>> +    {
>> +      int saved_errno = errno;
>> +      fclose (fp);
>> +      __set_errno (saved_errno);
>> +    }
>> +
>>    return conf;
>>  }
>
> Why not close the FILE on __resolv_conf_load? It make the FILE object 
> cleanup as close as its creation, which usually improves readability.

Sorry, I don't understand.  Isn't this what the patch does?

Thanks,
Florian

Re: [PATCH 3/5] resolv: Fix file handle leak in __resolv_conf_load [BZ #25429]

[Adhemerval Zanella]

On 13/02/2020 18:08, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 21/01/2020 15:41, Florian Weimer wrote:
>>> res_vinit_1 did not close the stream on errors, only on success.
>>> This change moves closing the stream to __resolv_conf_load, for both
>>> the success and error cases.
>>>
>>> Fixes commit 89f187a40fc0ad4e22838526bfe34d73f758b776 ("resolv: Use
>>> getline for configuration file reading in res_vinit_1") and commit
>>> 3f853f22c87f0b671c0366eb290919719fa56c0e ("resolv: Lift domain search
>>> list limits [BZ #19569] [BZ #21475]"), where memory allocation was
>>> introduced into res_vinit_1.
>>> ---
>>>  resolv/res_init.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/resolv/res_init.c b/resolv/res_init.c
>>> index 95dce098aa..09345718cd 100644
>>> --- a/resolv/res_init.c
>>> +++ b/resolv/res_init.c
>>> @@ -508,7 +508,6 @@ res_vinit_1 (FILE *fp, struct resolv_conf_parser *parser)
>>>                continue;
>>>              }
>>>          }
>>> -      fclose (fp);
>>>      }
>>>    if (__glibc_unlikely (nameserver_list_size (&parser->nameserver_list) == 0))
>>>      {
>>> @@ -593,6 +592,13 @@ __resolv_conf_load (struct __res_state *preinit)
>>>      }
>>>    resolv_conf_parser_free (&parser);
>>>  
>>> +  if (fp != NULL)
>>> +    {
>>> +      int saved_errno = errno;
>>> +      fclose (fp);
>>> +      __set_errno (saved_errno);
>>> +    }
>>> +
>>>    return conf;
>>>  }
>>
>> Why not close the FILE on __resolv_conf_load? It make the FILE object 
>> cleanup as close as its creation, which usually improves readability.
> 
> Sorry, I don't understand.  Isn't this what the patch does?

Nevermind, for some reason I though it was doing on res_vinit_1.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
 

[PATCH 2/5] resolv: Use <file_change_detection.h> in __resolv_conf_get_current

[Florian Weimer]

Only minor functional changes (i.e., regarding the handling of
directories, which are now treated as empty files).
---
 resolv/resolv_conf.c | 43 ++++++++-----------------------------------
 1 file changed, 8 insertions(+), 35 deletions(-)

diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
index 08c50ef19e..d954ba9a5a 100644
--- a/resolv/resolv_conf.c
+++ b/resolv/resolv_conf.c
@@ -24,6 +24,7 @@
 #include <resolv-internal.h>
 #include <sys/stat.h>
 #include <libc-symbols.h>
+#include <file_change_detection.h>
 
 /* _res._u._ext.__glibc_extension_index is used as an index into a
    struct resolv_conf_array object.  The intent of this construction
@@ -68,12 +69,8 @@ struct resolv_conf_global
   /* Cached current configuration object for /etc/resolv.conf.  */
   struct resolv_conf *conf_current;
 
-  /* These properties of /etc/resolv.conf are used to check if the
-     configuration needs reloading.  */
-  struct timespec conf_mtime;
-  struct timespec conf_ctime;
-  off64_t conf_size;
-  ino64_t conf_ino;
+  /* File system identification for /etc/resolv.conf.  */
+  struct file_change_detection file_resolve_conf;
 };
 
 /* Lazily allocated storage for struct resolv_conf_global.  */
@@ -123,37 +120,16 @@ conf_decrement (struct resolv_conf *conf)
 struct resolv_conf *
 __resolv_conf_get_current (void)
 {
-  struct stat64 st;
-  if (stat64 (_PATH_RESCONF, &st) != 0)
-    {
-    switch (errno)
-      {
-      case EACCES:
-      case EISDIR:
-      case ELOOP:
-      case ENOENT:
-      case ENOTDIR:
-      case EPERM:
-        /* Ignore errors due to file system contents.  */
-        memset (&st, 0, sizeof (st));
-        break;
-      default:
-        /* Other errors are fatal.  */
-        return NULL;
-      }
-    }
+  struct file_change_detection initial;
+  if (!file_change_detection_for_path (&initial, _PATH_RESCONF))
+    return NULL;
 
   struct resolv_conf_global *global_copy = get_locked_global ();
   if (global_copy == NULL)
     return NULL;
   struct resolv_conf *conf;
   if (global_copy->conf_current != NULL
-      && (global_copy->conf_mtime.tv_sec == st.st_mtim.tv_sec
-          && global_copy->conf_mtime.tv_nsec == st.st_mtim.tv_nsec
-          && global_copy->conf_ctime.tv_sec == st.st_ctim.tv_sec
-          && global_copy->conf_ctime.tv_nsec == st.st_ctim.tv_nsec
-          && global_copy->conf_ino == st.st_ino
-          && global_copy->conf_size == st.st_size))
+      && file_is_unchanged (&initial, &global_copy->file_resolve_conf))
     /* We can reuse the cached configuration object.  */
     conf = global_copy->conf_current;
   else
@@ -171,10 +147,7 @@ __resolv_conf_get_current (void)
              read could be a newer version of the file, but this does
              not matter because this will lead to an extraneous reload
              later.  */
-          global_copy->conf_mtime = st.st_mtim;
-          global_copy->conf_ctime = st.st_ctim;
-          global_copy->conf_ino = st.st_ino;
-          global_copy->conf_size = st.st_size;
+          global_copy->file_resolve_conf = initial;
         }
     }
 
-- 
2.24.1

Re: [PATCH 2/5] resolv: Use <file_change_detection.h> in __resolv_conf_get_current

[Adhemerval Zanella]

On 21/01/2020 15:41, Florian Weimer wrote:
> Only minor functional changes (i.e., regarding the handling of
> directories, which are now treated as empty files).

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  resolv/resolv_conf.c | 43 ++++++++-----------------------------------
>  1 file changed, 8 insertions(+), 35 deletions(-)
> 
> diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
> index 08c50ef19e..d954ba9a5a 100644
> --- a/resolv/resolv_conf.c
> +++ b/resolv/resolv_conf.c
> @@ -24,6 +24,7 @@
>  #include <resolv-internal.h>
>  #include <sys/stat.h>
>  #include <libc-symbols.h>
> +#include <file_change_detection.h>
>  
>  /* _res._u._ext.__glibc_extension_index is used as an index into a
>     struct resolv_conf_array object.  The intent of this construction
> @@ -68,12 +69,8 @@ struct resolv_conf_global
>    /* Cached current configuration object for /etc/resolv.conf.  */
>    struct resolv_conf *conf_current;
>  
> -  /* These properties of /etc/resolv.conf are used to check if the
> -     configuration needs reloading.  */
> -  struct timespec conf_mtime;
> -  struct timespec conf_ctime;
> -  off64_t conf_size;
> -  ino64_t conf_ino;
> +  /* File system identification for /etc/resolv.conf.  */
> +  struct file_change_detection file_resolve_conf;
>  };
>  
>  /* Lazily allocated storage for struct resolv_conf_global.  */

Ok.

> @@ -123,37 +120,16 @@ conf_decrement (struct resolv_conf *conf)
>  struct resolv_conf *
>  __resolv_conf_get_current (void)
>  {
> -  struct stat64 st;
> -  if (stat64 (_PATH_RESCONF, &st) != 0)
> -    {
> -    switch (errno)
> -      {
> -      case EACCES:
> -      case EISDIR:
> -      case ELOOP:
> -      case ENOENT:
> -      case ENOTDIR:
> -      case EPERM:
> -        /* Ignore errors due to file system contents.  */
> -        memset (&st, 0, sizeof (st));
> -        break;
> -      default:
> -        /* Other errors are fatal.  */
> -        return NULL;
> -      }
> -    }
> +  struct file_change_detection initial;
> +  if (!file_change_detection_for_path (&initial, _PATH_RESCONF))
> +    return NULL;
>  

Ok.

>    struct resolv_conf_global *global_copy = get_locked_global ();
>    if (global_copy == NULL)
>      return NULL;
>    struct resolv_conf *conf;
>    if (global_copy->conf_current != NULL
> -      && (global_copy->conf_mtime.tv_sec == st.st_mtim.tv_sec
> -          && global_copy->conf_mtime.tv_nsec == st.st_mtim.tv_nsec
> -          && global_copy->conf_ctime.tv_sec == st.st_ctim.tv_sec
> -          && global_copy->conf_ctime.tv_nsec == st.st_ctim.tv_nsec
> -          && global_copy->conf_ino == st.st_ino
> -          && global_copy->conf_size == st.st_size))
> +      && file_is_unchanged (&initial, &global_copy->file_resolve_conf))
>      /* We can reuse the cached configuration object.  */
>      conf = global_copy->conf_current;
>    else
> @@ -171,10 +147,7 @@ __resolv_conf_get_current (void)
>               read could be a newer version of the file, but this does
>               not matter because this will lead to an extraneous reload
>               later.  */
> -          global_copy->conf_mtime = st.st_mtim;
> -          global_copy->conf_ctime = st.st_ctim;
> -          global_copy->conf_ino = st.st_ino;
> -          global_copy->conf_size = st.st_size;
> +          global_copy->file_resolve_conf = initial;
>          }
>      }
>  
> 

Ok.

[PATCH 5/5] resolv: Fix ABA race in /etc/resolv.conf change detection [BZ #25420]

[Florian Weimer]

__resolv_conf_get_current should only record the initial file
change data if after verifying that file just read matches the
original measurement.  Fixes commit aef16cc8a4c670036d45590877
("resolv: Automatically reload a changed /etc/resolv.conf file
[BZ #984]").
---
 resolv/resolv_conf.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
index bdd2ebb909..29a1f4fb94 100644
--- a/resolv/resolv_conf.c
+++ b/resolv/resolv_conf.c
@@ -136,18 +136,25 @@ __resolv_conf_get_current (void)
     {
       /* Parse configuration while holding the lock.  This avoids
          duplicate work.  */
-      conf = __resolv_conf_load (NULL, NULL);
+      struct file_change_detection after_load;
+      conf = __resolv_conf_load (NULL, &after_load);
       if (conf != NULL)
         {
           if (global_copy->conf_current != NULL)
             conf_decrement (global_copy->conf_current);
           global_copy->conf_current = conf; /* Takes ownership.  */
 
-          /* Update file modification stamps.  The configuration we
-             read could be a newer version of the file, but this does
-             not matter because this will lead to an extraneous reload
-             later.  */
-          global_copy->file_resolve_conf = initial;
+          /* Update file change detection data, but only if it matches
+             the initial measurement.  This avoids an ABA race in case
+             /etc/resolv.conf is temporarily replaced while the file
+             is read (after the initial measurement), and restored to
+             the initial version later.  */
+          if (file_is_unchanged (&initial, &after_load))
+            global_copy->file_resolve_conf = after_load;
+          else
+            /* If there is a discrepancy, trigger a reload during the
+               next use.  */
+            global_copy->file_resolve_conf.size = -1;
         }
     }
 
-- 
2.24.1

Re: [PATCH 5/5] resolv: Fix ABA race in /etc/resolv.conf change detection [BZ #25420]

[Adhemerval Zanella]

On 21/01/2020 15:42, Florian Weimer wrote:
> __resolv_conf_get_current should only record the initial file
> change data if after verifying that file just read matches the
> original measurement.  Fixes commit aef16cc8a4c670036d45590877
> ("resolv: Automatically reload a changed /etc/resolv.conf file
> [BZ #984]").

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  resolv/resolv_conf.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/resolv/resolv_conf.c b/resolv/resolv_conf.c
> index bdd2ebb909..29a1f4fb94 100644
> --- a/resolv/resolv_conf.c
> +++ b/resolv/resolv_conf.c
> @@ -136,18 +136,25 @@ __resolv_conf_get_current (void)
>      {
>        /* Parse configuration while holding the lock.  This avoids
>           duplicate work.  */
> -      conf = __resolv_conf_load (NULL, NULL);
> +      struct file_change_detection after_load;
> +      conf = __resolv_conf_load (NULL, &after_load);
>        if (conf != NULL)
>          {
>            if (global_copy->conf_current != NULL)
>              conf_decrement (global_copy->conf_current);
>            global_copy->conf_current = conf; /* Takes ownership.  */
>  
> -          /* Update file modification stamps.  The configuration we
> -             read could be a newer version of the file, but this does
> -             not matter because this will lead to an extraneous reload
> -             later.  */
> -          global_copy->file_resolve_conf = initial;
> +          /* Update file change detection data, but only if it matches
> +             the initial measurement.  This avoids an ABA race in case
> +             /etc/resolv.conf is temporarily replaced while the file
> +             is read (after the initial measurement), and restored to
> +             the initial version later.  */
> +          if (file_is_unchanged (&initial, &after_load))
> +            global_copy->file_resolve_conf = after_load;
> +          else
> +            /* If there is a discrepancy, trigger a reload during the
> +               next use.  */
> +            global_copy->file_resolve_conf.size = -1;
>          }
>      }
>  
> 

Ok.