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

  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.
  

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.