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

Message ID c657cfe9aedbc48a368f11442f453190ed1cf1e9.1579631655.git.fweimer@redhat.com
State New
Headers show
Series
  • Race condition in /etc/resolv.conf reloading (bug 25420)
Related show

Commit Message

Florian Weimer Jan. 21, 2020, 6:41 p.m.
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(-)

-- 
2.24.1

Comments

Adhemerval Zanella Feb. 13, 2020, 8:53 p.m. | #1
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

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;
         }
     }