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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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.