8a8cfb
commit d4625a19fe64f664119a541b317fb83de01bb273
8a8cfb
Author: Florian Weimer <fweimer@redhat.com>
8a8cfb
Date:   Tue Nov 12 12:25:49 2019 +0100
8a8cfb
8a8cfb
    login: Use pread64 in utmp implementation
8a8cfb
    
8a8cfb
    This reduces the possible error scenarios considerably because
8a8cfb
    no longer can file seek fail, leaving the file descriptor in an
8a8cfb
    inconsistent state and out of sync with the cache.
8a8cfb
    
8a8cfb
    As a result, it is possible to avoid setting file_offset to -1
8a8cfb
    to make an error persistent.  Instead, subsequent calls will retry
8a8cfb
    the operation and report any errors returned by the kernel.
8a8cfb
    
8a8cfb
    This change also avoids reading the file from the start if pututline
8a8cfb
    is called multiple times, to work around lock acquisition failures
8a8cfb
    due to timeouts.
8a8cfb
    
8a8cfb
    Change-Id: If21ea0c162c38830a89331ea93cddec14c0974de
8a8cfb
8a8cfb
diff --git a/login/utmp_file.c b/login/utmp_file.c
8a8cfb
index e653d14967c4fb7a..c828a28ac54c150e 100644
8a8cfb
--- a/login/utmp_file.c
8a8cfb
+++ b/login/utmp_file.c
8a8cfb
@@ -162,12 +162,35 @@ maybe_setutent (void)
8a8cfb
   return file_fd >= 0 || __libc_setutent ();
8a8cfb
 }
8a8cfb
 
8a8cfb
+/* Reads the entry at file_offset, storing it in last_entry and
8a8cfb
+   updating file_offset on success.  Returns -1 for a read error, 0
8a8cfb
+   for EOF, and 1 for a successful read.  last_entry and file_offset
8a8cfb
+   are only updated on a successful and complete read.  */
8a8cfb
+static ssize_t
8a8cfb
+read_last_entry (void)
8a8cfb
+{
8a8cfb
+  struct utmp buffer;
8a8cfb
+  ssize_t nbytes = __pread64_nocancel (file_fd, &buffer, sizeof (buffer),
8a8cfb
+				       file_offset);
8a8cfb
+  if (nbytes < 0)
8a8cfb
+    return -1;
8a8cfb
+  else if (nbytes != sizeof (buffer))
8a8cfb
+    /* Assume EOF.  */
8a8cfb
+    return 0;
8a8cfb
+  else
8a8cfb
+    {
8a8cfb
+      last_entry = buffer;
8a8cfb
+      file_offset += sizeof (buffer);
8a8cfb
+      return 1;
8a8cfb
+    }
8a8cfb
+}
8a8cfb
+
8a8cfb
 int
8a8cfb
 __libc_getutent_r (struct utmp *buffer, struct utmp **result)
8a8cfb
 {
8a8cfb
-  ssize_t nbytes;
8a8cfb
+  int saved_errno = errno;
8a8cfb
 
8a8cfb
-  if (!maybe_setutent () || file_offset == -1l)
8a8cfb
+  if (!maybe_setutent ())
8a8cfb
     {
8a8cfb
       /* Not available.  */
8a8cfb
       *result = NULL;
8a8cfb
@@ -175,25 +198,22 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
8a8cfb
     }
8a8cfb
 
8a8cfb
   if (try_file_lock (file_fd, F_RDLCK))
8a8cfb
-    nbytes = 0;
8a8cfb
-  else
8a8cfb
-    {
8a8cfb
-      /* Read the next entry.  */
8a8cfb
-      nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp));
8a8cfb
-      file_unlock (file_fd);
8a8cfb
-    }
8a8cfb
+    return -1;
8a8cfb
 
8a8cfb
-  if (nbytes != sizeof (struct utmp))
8a8cfb
+  ssize_t nbytes = read_last_entry ();
8a8cfb
+  file_unlock (file_fd);
8a8cfb
+
8a8cfb
+  if (nbytes <= 0)		/* Read error or EOF.  */
8a8cfb
     {
8a8cfb
-      if (nbytes != 0)
8a8cfb
-	file_offset = -1l;
8a8cfb
+      if (nbytes == 0)
8a8cfb
+	/* errno should be unchanged to indicate success.  A premature
8a8cfb
+	   EOF is treated like an EOF (missing complete record at the
8a8cfb
+	   end).  */
8a8cfb
+	__set_errno (saved_errno);
8a8cfb
       *result = NULL;
8a8cfb
       return -1;
8a8cfb
     }
8a8cfb
 
8a8cfb
-  /* Update position pointer.  */
8a8cfb
-  file_offset += sizeof (struct utmp);
8a8cfb
-
8a8cfb
   memcpy (buffer, &last_entry, sizeof (struct utmp));
8a8cfb
   *result = buffer;
8a8cfb
 
8a8cfb
@@ -209,15 +229,15 @@ internal_getut_nolock (const struct utmp *id)
8a8cfb
 {
8a8cfb
   while (1)
8a8cfb
     {
8a8cfb
-      /* Read the next entry.  */
8a8cfb
-      if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
8a8cfb
-	  != sizeof (struct utmp))
8a8cfb
+      ssize_t nbytes = read_last_entry ();
8a8cfb
+      if (nbytes < 0)
8a8cfb
+	return -1;
8a8cfb
+      if (nbytes == 0)
8a8cfb
 	{
8a8cfb
+	  /* End of file reached.  */
8a8cfb
 	  __set_errno (ESRCH);
8a8cfb
-	  file_offset = -1l;
8a8cfb
 	  return -1;
8a8cfb
 	}
8a8cfb
-      file_offset += sizeof (struct utmp);
8a8cfb
 
8a8cfb
       if (matches_last_entry (id))
8a8cfb
 	break;
8a8cfb
@@ -249,7 +269,7 @@ int
8a8cfb
 __libc_getutid_r (const struct utmp *id, struct utmp *buffer,
8a8cfb
 		  struct utmp **result)
8a8cfb
 {
8a8cfb
-  if (!maybe_setutent () || file_offset == -1l)
8a8cfb
+  if (!maybe_setutent ())
8a8cfb
     {
8a8cfb
       *result = NULL;
8a8cfb
       return -1;
8a8cfb
@@ -276,7 +296,7 @@ int
8a8cfb
 __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
8a8cfb
 		    struct utmp **result)
8a8cfb
 {
8a8cfb
-  if (!maybe_setutent () || file_offset == -1l)
8a8cfb
+  if (!maybe_setutent ())
8a8cfb
     {
8a8cfb
       *result = NULL;
8a8cfb
       return -1;
8a8cfb
@@ -290,16 +310,21 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
8a8cfb
 
8a8cfb
   while (1)
8a8cfb
     {
8a8cfb
-      /* Read the next entry.  */
8a8cfb
-      if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
8a8cfb
-	  != sizeof (struct utmp))
8a8cfb
+      ssize_t nbytes = read_last_entry ();
8a8cfb
+      if (nbytes < 0)
8a8cfb
 	{
8a8cfb
+	  file_unlock (file_fd);
8a8cfb
+	  *result = NULL;
8a8cfb
+	  return -1;
8a8cfb
+	}
8a8cfb
+      if (nbytes == 0)
8a8cfb
+	{
8a8cfb
+	  /* End of file reached.  */
8a8cfb
+	  file_unlock (file_fd);
8a8cfb
 	  __set_errno (ESRCH);
8a8cfb
-	  file_offset = -1l;
8a8cfb
 	  *result = NULL;
8a8cfb
-	  goto unlock_return;
8a8cfb
+	  return -1;
8a8cfb
 	}
8a8cfb
-      file_offset += sizeof (struct utmp);
8a8cfb
 
8a8cfb
       /* Stop if we found a user or login entry.  */
8a8cfb
       if ((last_entry.ut_type == USER_PROCESS
8a8cfb
@@ -309,20 +334,18 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer,
8a8cfb
 	break;
8a8cfb
     }
8a8cfb
 
8a8cfb
+  file_unlock (file_fd);
8a8cfb
   memcpy (buffer, &last_entry, sizeof (struct utmp));
8a8cfb
   *result = buffer;
8a8cfb
 
8a8cfb
-unlock_return:
8a8cfb
-  file_unlock (file_fd);
8a8cfb
-
8a8cfb
-  return ((*result == NULL) ? -1 : 0);
8a8cfb
+  return 0;
8a8cfb
 }
8a8cfb
 
8a8cfb
 
8a8cfb
 struct utmp *
8a8cfb
 __libc_pututline (const struct utmp *data)
8a8cfb
 {
8a8cfb
-  if (!maybe_setutent () || file_offset == -1l)
8a8cfb
+  if (!maybe_setutent ())
8a8cfb
     return NULL;
8a8cfb
 
8a8cfb
   struct utmp *pbuf;
8a8cfb
@@ -337,8 +360,7 @@ __libc_pututline (const struct utmp *data)
8a8cfb
       if (new_fd == -1)
8a8cfb
 	return NULL;
8a8cfb
 
8a8cfb
-      if (__lseek64 (new_fd, __lseek64 (file_fd, 0, SEEK_CUR), SEEK_SET) == -1
8a8cfb
-	  || __dup2 (new_fd, file_fd) < 0)
8a8cfb
+      if (__dup2 (new_fd, file_fd) < 0)
8a8cfb
 	{
8a8cfb
 	  __close_nocancel_nostatus (new_fd);
8a8cfb
 	  return NULL;
8a8cfb
@@ -355,69 +377,70 @@ __libc_pututline (const struct utmp *data)
8a8cfb
   bool found = false;
8a8cfb
   if (matches_last_entry (data))
8a8cfb
     {
8a8cfb
-      if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0)
8a8cfb
+      /* Read back the entry under the write lock.  */
8a8cfb
+      file_offset -= sizeof (last_entry);
8a8cfb
+      ssize_t nbytes = read_last_entry ();
8a8cfb
+      if (nbytes < 0)
8a8cfb
 	{
8a8cfb
 	  file_unlock (file_fd);
8a8cfb
 	  return NULL;
8a8cfb
 	}
8a8cfb
-      if (__read_nocancel (file_fd, &last_entry, sizeof (last_entry))
8a8cfb
-	  != sizeof (last_entry))
8a8cfb
-	{
8a8cfb
-	  if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0)
8a8cfb
-	    {
8a8cfb
-	      file_unlock (file_fd);
8a8cfb
-	      return NULL;
8a8cfb
-	    }
8a8cfb
-	  found = false;
8a8cfb
-	}
8a8cfb
+
8a8cfb
+      if (nbytes == 0)
8a8cfb
+	/* End of file reached.  */
8a8cfb
+	found = false;
8a8cfb
       else
8a8cfb
 	found = matches_last_entry (data);
8a8cfb
     }
8a8cfb
 
8a8cfb
   if (!found)
8a8cfb
+    /* Search forward for the entry.  */
8a8cfb
     found = internal_getut_nolock (data) >= 0;
8a8cfb
 
8a8cfb
+  off64_t write_offset;
8a8cfb
   if (!found)
8a8cfb
     {
8a8cfb
       /* We append the next entry.  */
8a8cfb
-      file_offset = __lseek64 (file_fd, 0, SEEK_END);
8a8cfb
-      if (file_offset % sizeof (struct utmp) != 0)
8a8cfb
-	{
8a8cfb
-	  file_offset -= file_offset % sizeof (struct utmp);
8a8cfb
-	  __ftruncate64 (file_fd, file_offset);
8a8cfb
-
8a8cfb
-	  if (__lseek64 (file_fd, 0, SEEK_END) < 0)
8a8cfb
-	    {
8a8cfb
-	      pbuf = NULL;
8a8cfb
-	      goto unlock_return;
8a8cfb
-	    }
8a8cfb
-	}
8a8cfb
+      write_offset = __lseek64 (file_fd, 0, SEEK_END);
8a8cfb
+
8a8cfb
+      /* Round down to the next multiple of the entry size.  This
8a8cfb
+	 ensures any partially-written record is overwritten by the
8a8cfb
+	 new record.  */
8a8cfb
+      write_offset = (write_offset / sizeof (struct utmp)
8a8cfb
+		      * sizeof (struct utmp));
8a8cfb
     }
8a8cfb
   else
8a8cfb
+    /* Overwrite last_entry.  */
8a8cfb
+    write_offset = file_offset - sizeof (struct utmp);
8a8cfb
+
8a8cfb
+  /* Write the new data.  */
8a8cfb
+  ssize_t nbytes;
8a8cfb
+  if (__lseek64 (file_fd, write_offset, SEEK_SET) < 0
8a8cfb
+      || (nbytes = __write_nocancel (file_fd, data, sizeof (struct utmp))) < 0)
8a8cfb
     {
8a8cfb
-      /* We replace the just read entry.  */
8a8cfb
-      file_offset -= sizeof (struct utmp);
8a8cfb
-      __lseek64 (file_fd, file_offset, SEEK_SET);
8a8cfb
+      /* There is no need to recover the file position because all
8a8cfb
+	 reads use pread64, and any future write is preceded by
8a8cfb
+	 another seek.  */
8a8cfb
+      file_unlock (file_fd);
8a8cfb
+      return NULL;
8a8cfb
     }
8a8cfb
 
8a8cfb
-  /* Write the new data.  */
8a8cfb
-  if (__write_nocancel (file_fd, data, sizeof (struct utmp))
8a8cfb
-      != sizeof (struct utmp))
8a8cfb
+  if (nbytes != sizeof (struct utmp))
8a8cfb
     {
8a8cfb
       /* If we appended a new record this is only partially written.
8a8cfb
 	 Remove it.  */
8a8cfb
       if (!found)
8a8cfb
-	(void) __ftruncate64 (file_fd, file_offset);
8a8cfb
-      pbuf = NULL;
8a8cfb
-    }
8a8cfb
-  else
8a8cfb
-    {
8a8cfb
-      file_offset += sizeof (struct utmp);
8a8cfb
-      pbuf = (struct utmp *) data;
8a8cfb
+	(void) __ftruncate64 (file_fd, write_offset);
8a8cfb
+      file_unlock (file_fd);
8a8cfb
+      /* Assume that the write failure was due to missing disk
8a8cfb
+	 space.  */
8a8cfb
+      __set_errno (ENOSPC);
8a8cfb
+      return NULL;
8a8cfb
     }
8a8cfb
 
8a8cfb
- unlock_return:
8a8cfb
   file_unlock (file_fd);
8a8cfb
+  file_offset = write_offset + sizeof (struct utmp);
8a8cfb
+  pbuf = (struct utmp *) data;
8a8cfb
 
8a8cfb
   return pbuf;
8a8cfb
 }