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

Message ID 95b81fd1ccd9d41c4f6a897a1c43d1298da0486e.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:42 p.m.
__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(-)

-- 
2.24.1

Comments

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

Patch

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