Re: [PATCH] login: pututxline could fail to overwrite existing entries [BZ #24902]

["Gabriel F. T. Gomes" <gabriel at inconstante dot net dot br>]

On Wed, Aug 21 2019, Florian Weimer wrote:
> 
> Previously, if pututxline could not upgrade the read lock to a
> write lock, internal_getut_r would update file_offset only,
> without updating last_entry, and a subsequent call would not
> overwrite the existing utmpx entry at file_offset, instead
> creating a new entry.  This has been observed to cause unbounded
> file growth in high-load situations.

Looks good to me with the typo below fixed.  Thanks.

Reviewed-by: Gabriel F. T. Gomes <gabrielftg@linux.ibm.com>

> +/* Create the initial entry in a subprocess, so that the utmp
> +   subsystem in the original rocess is not disturbed.  */
                                ~~~~~~
Typo: process?

> +__attribute__ ((noreturn)) static void
> +subprocess_lock_file (void)
> +{
> +  int fd = xopen (path, O_RDONLY, 0);
> +
> +  struct flock64 fl =
> +    {
> +     .l_type = F_RDLCK,
> +     fl.l_whence = SEEK_SET,
> +    };
> +  TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0);
> +
> +  /* Signal to the main process that the lock has been acquired.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Wait for the unlock request from the main process.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Implicitly unlock the file.  */
> +  xclose (fd);
> +
> +  /* Overwrite the existing entry.  */
> +  TEST_COMPARE (utmpname (path), 0);
> +  errno = 0;
> +  setutxent ();
> +  TEST_COMPARE (errno, 0);
> +  TEST_VERIFY (write_entry (102) != NULL);

OK.  This will replace the only entry in the utmp file.

(that's my first contact with the user management database file and
functions, so I'll not 100% sure I got it right).

> +static int
> +do_test (void)
> +{
> +  xclose (create_temp_file ("tst-pututxline-lockfail-", &path));
> +
> +  {
> +    pthread_barrierattr_t attr;
> +    xpthread_barrierattr_init (&attr);
> +    xpthread_barrierattr_setpshared (&attr, PTHREAD_SCOPE_PROCESS);
> +    barrier = support_shared_allocate (sizeof (*barrier));
> +    xpthread_barrier_init (barrier, &attr, 2);
> +    xpthread_barrierattr_destroy (&attr);
> +  }
> +
> +  /* Write the initial entry.  */
> +  support_isolate_in_subprocess (subprocess_create_entry, NULL);
> +
> +  pid_t locker_pid = xfork ();
> +  if (locker_pid == 0)
> +    subprocess_lock_file ();
> +
> +  /* Wait for the file locking to complete.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Try to add another entry.  This attempt will fail, with EINTR or
> +     EAGAIN.  */
> +  TEST_COMPARE (utmpname (path), 0);
> +  TEST_VERIFY (write_entry (102) == NULL);
> +  if (errno != EINTR)
> +    TEST_COMPARE (errno, EAGAIN);

OK.  This will try to update the current entry, leaving file_offset and
last_entry in an inconsistent state (the bug this patch fixes).

> +  /* Signal the subprocess to overwrite the entry.  */
> +  xpthread_barrier_wait (barrier);
> +
> +  /* Wait for write and unlock to complete.  */
> +  {
> +    int status;
> +    xwaitpid (locker_pid, &status, 0);
> +    TEST_COMPARE (status, 0);
> +  }
> +
> +  /* The file is no longer locked, so this operation will succeed.  */
> +  TEST_VERIFY (write_entry (103) != NULL);
> +  errno = 0;
> +  endutxent ();
> +  TEST_COMPARE (errno, 0);
> +
> +  /* Check that there is just one entry with the expected contents.
> +     If pututxline becomes desynchronized internally, the entry is not
> +     overwritten (bug 24902).  */
> +  errno = 0;
> +  setutxent ();
> +  TEST_COMPARE (errno, 0);
> +  struct utmpx *ut = getutxent ();
> +  TEST_VERIFY_EXIT (ut != NULL);
> +  TEST_COMPARE (ut->ut_type, LOGIN_PROCESS);
> +  TEST_COMPARE_STRING (ut->ut_id, "1");
> +  TEST_COMPARE_STRING (ut->ut_user, "root");
> +  TEST_COMPARE (ut->ut_pid, 103);

Without the fix, this assert fails, because getutxent finds the
not-overwritten entry with pid == 102.  After the fix, it works
correctly (note: I tested the patch with and without the actual fix).

> +  TEST_COMPARE_STRING (ut->ut_line, "entry");
> +  TEST_COMPARE_STRING (ut->ut_host, "localhost");
> +  TEST_VERIFY (getutxent () == NULL);

Without the fix, this assert fails, because there is an extra entry in
the database file.