Re: [PATCH] login: Disarm timer after utmp lock acquisition [BZ #24879]

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

On 12/08/2019 06:56, Florian Weimer wrote:
> If the file processing takes a long time for some reason, SIGALRM can
> arrive while the file is still being processed.  At that point, file
> access will fail with EINTR.  Disarming the timer after lock
> acquisition avoids that.  (If there was a previous alarm, it is the
> responsibility of the caller to deal with the EINTR error.)
> 
> 2019-08-05  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #24879]
> 	login: Disarm timer after utmp lock acquisition.
> 	* login/utmp_file.c (struct file_locking): Remove.
> 	(file_locking_failed): Adjust.
> 	(file_locking_restore): Remove function.
> 	(__libc_getutent_r): .
> 	(internal_getut_r): Likewise.
> 	(__libc_getutline_r): Likewise.
> 	(__libc_pututline): Likewise.
> 	(__libc_updwtmp): Likewise.

LGTM, with a nit below.

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


> 
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 3f21de2b71..9c64ebf63c 100644
> --- a/login/utmp_file.c
> +++ b/login/utmp_file.c
> @@ -53,35 +53,25 @@ static struct utmp last_entry;
>  static void timeout_handler (int signum) {};
>  
>  
> -
>  /* file_locking_failed (LOCKING, FD, TYPE) returns true if the locking
>     operation failed and recovery needs to be performed.
> -   (file_locking_restore (LOCKING) still needs to be called.)
>  
>     file_locking_unlock (FD) removes the lock (which must have been
> -   acquired).
> -
> -   file_locking_restore (LOCKING) is needed to clean up in both
> -   cases.  */
> -
> -struct file_locking
> -{
> -  struct sigaction old_action;
> -  unsigned int old_timeout;
> -};
> +   successfully acquired). */
>  
>  static bool
> -file_locking_failed (struct file_locking *locking, int fd, int type)
> +file_locking_failed (int fd, int type)
>  {
>    /* Cancel any existing alarm.  */
> -  locking->old_timeout = alarm (0);
> +  int old_timeout = alarm (0);
>  
>    /* Establish signal handler.  */
> +  struct sigaction old_action;
>    struct sigaction action;
>    action.sa_handler = timeout_handler;
>    __sigemptyset (&action.sa_mask);
>    action.sa_flags = 0;
> -  __sigaction (SIGALRM, &action, &locking->old_action);
> +  __sigaction (SIGALRM, &action, &old_action);
>  
>    alarm (TIMEOUT);
>  

Ok.

> @@ -91,7 +81,23 @@ file_locking_failed (struct file_locking *locking, int fd, int type)
>      .l_type = type,
>      fl.l_whence = SEEK_SET,
>     };
> - return __fcntl64_nocancel (fd, F_SETLKW, &fl) < 0;
> +
> + int status = __fcntl64_nocancel (fd, F_SETLKW, &fl) < 0;

I think it is better to change the status type to bool.

> + int saved_errno = errno;
> +
> + /* Reset the signal handler and alarm.  We must reset the alarm
> +    before resetting the handler so our alarm does not generate a
> +    spurious SIGALRM seen by the user.  However, we cannot just set
> +    the user's old alarm before restoring the handler, because then
> +    it's possible our handler could catch the user alarm's SIGARLM and
> +    then the user would never see the signal he expected.  */
> +  alarm (0);
> +  __sigaction (SIGALRM, &old_action, NULL);
> +  if (old_timeout != 0)
> +    alarm (old_timeout);
> +
> +  __set_errno (saved_errno);
> +  return status;
>  }
>  
>  static void

Ok.

> @@ -104,21 +110,6 @@ file_locking_unlock (int fd)
>    __fcntl64_nocancel (fd, F_SETLKW, &fl);
>  }
>  
> -static void
> -file_locking_restore (struct file_locking *locking)
> -{
> -  /* Reset the signal handler and alarm.  We must reset the alarm
> -     before resetting the handler so our alarm does not generate a
> -     spurious SIGALRM seen by the user.  However, we cannot just set
> -     the user's old alarm before restoring the handler, because then
> -     it's possible our handler could catch the user alarm's SIGARLM
> -     and then the user would never see the signal he expected.  */
> -  alarm (0);
> -  __sigaction (SIGALRM, &locking->old_action, NULL);
> -  if (locking->old_timeout != 0)
> -    alarm (locking->old_timeout);
> -}
> -
>  #ifndef TRANSFORM_UTMP_FILE_NAME
>  # define TRANSFORM_UTMP_FILE_NAME(file_name) (file_name)
>  #endif

Ok.

> @@ -167,8 +158,7 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
>        return -1;
>      }
>  
> -  struct file_locking fl;
> -  if (file_locking_failed (&fl, file_fd, F_RDLCK))
> +  if (file_locking_failed (file_fd, F_RDLCK))
>      nbytes = 0;
>    else
>      {

Ok.

> @@ -176,7 +166,6 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
>        nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
>        file_locking_unlock (file_fd);
>      }
> -  file_locking_restore (&fl);
>  
>    if (nbytes != sizeof (struct utmp))
>      {

Ok.

> @@ -202,11 +191,9 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
>  {
>    int result = -1;
>  
> -  struct file_locking fl;
> -  if (file_locking_failed (&fl, file_fd, F_RDLCK))
> +  if (file_locking_failed (file_fd, F_RDLCK))
>      {
>        *lock_failed = true;
> -      file_locking_restore (&fl);
>        return -1;
>      }
>  

Ok.

> @@ -258,7 +245,6 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer,
>  
>  unlock_return:
>    file_locking_unlock (file_fd);
> -  file_locking_restore (&fl);
>  
>    return result;
>  }

Ok.

> @@ -304,11 +290,9 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
>        return -1;
>      }
>  
> -  struct file_locking fl;
> -  if (file_locking_failed (&fl, file_fd, F_RDLCK))
> +  if (file_locking_failed (file_fd, F_RDLCK))
>      {
>        *result = NULL;
> -      file_locking_restore (&fl);
>        return -1;
>      }
>  

Ok.

> @@ -338,7 +322,6 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
>  
>  unlock_return:
>    file_locking_unlock (file_fd);
> -  file_locking_restore (&fl);
>  
>    return ((*result == NULL) ? -1 : 0);
>  }

Ok.

> @@ -395,12 +378,8 @@ __libc_pututline (const struct utmp *data)
>  	}
>      }
>  
> -  struct file_locking fl;
> -  if (file_locking_failed (&fl, file_fd, F_WRLCK))
> -    {
> -      file_locking_restore (&fl);
> -      return NULL;
> -    }
> +  if (file_locking_failed (file_fd, F_WRLCK))
> +    return NULL;
>  
>    if (found < 0)
>      {

Ok.

> @@ -443,7 +422,6 @@ __libc_pututline (const struct utmp *data)
>  
>   unlock_return:
>    file_locking_unlock (file_fd);
> -  file_locking_restore (&fl);
>  
>    return pbuf;
>  }

Ok.

> @@ -472,10 +450,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
>    if (fd < 0)
>      return -1;
>  
> -  struct file_locking fl;
> -  if (file_locking_failed (&fl, fd, F_WRLCK))
> +  if (file_locking_failed (fd, F_WRLCK))
>      {
> -      file_locking_restore (&fl);
>        __close_nocancel_nostatus (fd);
>        return -1;
>      }

Ok.

> @@ -505,7 +481,6 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
>  
>  unlock_return:
>    file_locking_unlock (file_fd);
> -  file_locking_restore (&fl);
>  
>    /* Close WTMP file.  */
>    __close_nocancel_nostatus (fd);
> 

Ok.