From patchwork Tue Nov 12 11:50:11 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Adhemerval Zanella (Code Review)" X-Patchwork-Id: 1193484 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-106928-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=gnutoolchain-gerrit.osci.io Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="WVsHmmmm"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47C5fJ1xXqz9sNT for ; Tue, 12 Nov 2019 22:50:32 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:message-id:subject:references :reply-to:mime-version:content-transfer-encoding:content-type; q=dns; s=default; b=U8ze4xUGCXcHs+zW2DjmFy8RJgmO46zZPtc7TkuLw9r cIpJ3arjVfQ+2Dq9iHE51q0krI68TB4VG1gmmQ8v6DSYK+DmizmdPqb+Lwa3INZp TkGIdf7oryx1qpjYyfFqaC0ne1iQBWqr0gmAw6J1WbKXHNgZjv09Mn7zqiF3CrJ4 = DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:message-id:subject:references :reply-to:mime-version:content-transfer-encoding:content-type; s=default; bh=71bBdWivuTB8zMeGSFZIId3QvI8=; b=WVsHmmmmECBZv8sd2 LhZrau6owzKi4F89Yc07qLCxOPkDcS49TFKPDMEIZsv6vZ1594dzpCKyNshy6CWE /3JcrmlsMrl1ryuzv7C9kNzCWHZeurNIiFj2YWJOY9Vr7w6mLrfG+hUgT8Ljirnm Mn1hBoTgzZivGtZG8zYwOD+x1Q= Received: (qmail 107545 invoked by alias); 12 Nov 2019 11:50:18 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 107477 invoked by uid 89); 12 Nov 2019 11:50:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=acquisition X-HELO: mx1.osci.io X-Gerrit-PatchSet: 1 Date: Tue, 12 Nov 2019 06:50:11 -0500 From: "Florian Weimer (Code Review)" To: libc-alpha@sourceware.org Cc: Florian Weimer Message-ID: Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] login: Use pread64 in utmp implementation X-Gerrit-Change-Id: If21ea0c162c38830a89331ea93cddec14c0974de X-Gerrit-Change-Number: 615 X-Gerrit-ChangeURL: X-Gerrit-Commit: d6f3c228c5aa01739c447fe0d0d171283fbe2017 References: Reply-To: fweimer@redhat.com, fweimer@redhat.com, libc-alpha@sourceware.org MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/3.0.3-76-gf8b6da0ab5 Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/615 ...................................................................... login: Use pread64 in utmp implementation This reduces the possible error scenarios considerably because no longer can file seek fail, leaving the file descriptor in an inconsistent state and out of sync with the cache. As a result, it is possible to avoid setting file_offset to -1 to make an error persistent. Instead, subsequent calls will retry the operation and report any errors returned by the kernel. This change also avoids reading the file from the start if pututline is called multiple times, to work around lock acquisition failures due to timeouts. Change-Id: If21ea0c162c38830a89331ea93cddec14c0974de --- M login/utmp_file.c 1 file changed, 96 insertions(+), 73 deletions(-) diff --git a/login/utmp_file.c b/login/utmp_file.c index c41d17b..1b018b3 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -162,12 +162,35 @@ return file_fd >= 0 || __libc_setutent (); } +/* Reads the entry at file_offset, storing it in last_entry and + updating file_offset on success. Returns -1 for a read error, 0 + for EOF, and 1 for a successful read. last_entry and file_offset + are only updated on a successful and complete read. */ +static ssize_t +read_last_entry (void) +{ + struct utmp buffer; + ssize_t nbytes = __pread64_nocancel (file_fd, &buffer, sizeof (buffer), + file_offset); + if (nbytes < 0) + return -1; + else if (nbytes != sizeof (buffer)) + /* Assume EOF. */ + return 0; + else + { + last_entry = buffer; + file_offset += sizeof (buffer); + return 1; + } +} + int __libc_getutent_r (struct utmp *buffer, struct utmp **result) { - ssize_t nbytes; + int saved_errno = errno; - if (!maybe_setutent () || file_offset == -1l) + if (!maybe_setutent ()) { /* Not available. */ *result = NULL; @@ -175,25 +198,22 @@ } if (try_file_lock (file_fd, F_RDLCK)) - nbytes = 0; - else - { - /* Read the next entry. */ - nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp)); - file_unlock (file_fd); - } + return -1; - if (nbytes != sizeof (struct utmp)) + ssize_t nbytes = read_last_entry (); + file_unlock (file_fd); + + if (nbytes <= 0) /* Read error or EOF. */ { - if (nbytes != 0) - file_offset = -1l; + if (nbytes == 0) + /* errno should be unchanged to indicate success. A premature + EOF is treated like an EOF (missing complete record at the + end). */ + __set_errno (saved_errno); *result = NULL; return -1; } - /* Update position pointer. */ - file_offset += sizeof (struct utmp); - memcpy (buffer, &last_entry, sizeof (struct utmp)); *result = buffer; @@ -209,15 +229,15 @@ { while (1) { - /* Read the next entry. */ - if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp)) - != sizeof (struct utmp)) + ssize_t nbytes = read_last_entry (); + if (nbytes < 0) + return -1; + if (nbytes == 0) { + /* End of file reached. */ __set_errno (ESRCH); - file_offset = -1l; return -1; } - file_offset += sizeof (struct utmp); if (matches_last_entry (id)) break; @@ -249,7 +269,7 @@ __libc_getutid_r (const struct utmp *id, struct utmp *buffer, struct utmp **result) { - if (!maybe_setutent () || file_offset == -1l) + if (!maybe_setutent ()) { *result = NULL; return -1; @@ -276,7 +296,7 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer, struct utmp **result) { - if (!maybe_setutent () || file_offset == -1l) + if (!maybe_setutent ()) { *result = NULL; return -1; @@ -290,16 +310,21 @@ while (1) { - /* Read the next entry. */ - if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp)) - != sizeof (struct utmp)) + ssize_t nbytes = read_last_entry (); + if (nbytes < 0) { - __set_errno (ESRCH); - file_offset = -1l; + file_unlock (file_fd); *result = NULL; - goto unlock_return; + return -1; } - file_offset += sizeof (struct utmp); + if (nbytes == 0) + { + /* End of file reached. */ + file_unlock (file_fd); + __set_errno (ESRCH); + *result = NULL; + return -1; + } /* Stop if we found a user or login entry. */ if ((last_entry.ut_type == USER_PROCESS @@ -309,20 +334,18 @@ break; } + file_unlock (file_fd); memcpy (buffer, &last_entry, sizeof (struct utmp)); *result = buffer; -unlock_return: - file_unlock (file_fd); - - return ((*result == NULL) ? -1 : 0); + return 0; } struct utmp * __libc_pututline (const struct utmp *data) { - if (!maybe_setutent () || file_offset == -1l) + if (!maybe_setutent ()) return NULL; struct utmp *pbuf; @@ -337,8 +360,7 @@ if (new_fd == -1) return NULL; - if (__lseek64 (new_fd, __lseek64 (file_fd, 0, SEEK_CUR), SEEK_SET) == -1 - || __dup2 (new_fd, file_fd) < 0) + if (__dup2 (new_fd, file_fd) < 0) { __close_nocancel_nostatus (new_fd); return NULL; @@ -355,69 +377,70 @@ bool found = false; if (matches_last_entry (data)) { - if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0) + /* Read back the entry under the write lock. */ + file_offset -= sizeof (last_entry); + ssize_t nbytes = read_last_entry (); + if (nbytes < 0) { file_unlock (file_fd); return NULL; } - if (__read_nocancel (file_fd, &last_entry, sizeof (last_entry)) - != sizeof (last_entry)) - { - if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0) - { - file_unlock (file_fd); - return NULL; - } - found = false; - } + + if (nbytes == 0) + /* End of file reached. */ + found = false; else found = matches_last_entry (data); } if (!found) + /* Search forward for the entry. */ found = internal_getut_nolock (data) >= 0; + off64_t write_offset; if (!found) { /* We append the next entry. */ - file_offset = __lseek64 (file_fd, 0, SEEK_END); - if (file_offset % sizeof (struct utmp) != 0) - { - file_offset -= file_offset % sizeof (struct utmp); - __ftruncate64 (file_fd, file_offset); + write_offset = __lseek64 (file_fd, 0, SEEK_END); - if (__lseek64 (file_fd, 0, SEEK_END) < 0) - { - pbuf = NULL; - goto unlock_return; - } - } + /* Round down to the next multiple of the entry size. This + ensures any partially-written record is overwritten by the + new record. */ + write_offset = (write_offset / sizeof (struct utmp) + * sizeof (struct utmp)); } else - { - /* We replace the just read entry. */ - file_offset -= sizeof (struct utmp); - __lseek64 (file_fd, file_offset, SEEK_SET); - } + /* Overwrite last_entry. */ + write_offset = file_offset - sizeof (struct utmp); /* Write the new data. */ - if (__write_nocancel (file_fd, data, sizeof (struct utmp)) - != sizeof (struct utmp)) + ssize_t nbytes; + if (__lseek64 (file_fd, write_offset, SEEK_SET) < 0 + || (nbytes = __write_nocancel (file_fd, data, sizeof (struct utmp))) < 0) + { + /* There is no need to recover the file position because all + reads use pread64, and any future write is preceded by + another seek. */ + file_unlock (file_fd); + return NULL; + } + + if (nbytes != sizeof (struct utmp)) { /* If we appended a new record this is only partially written. Remove it. */ if (!found) - (void) __ftruncate64 (file_fd, file_offset); - pbuf = NULL; - } - else - { - file_offset += sizeof (struct utmp); - pbuf = (struct utmp *) data; + (void) __ftruncate64 (file_fd, write_offset); + file_unlock (file_fd); + /* Assume that the write failure was due to missing disk + space. */ + __set_errno (ENOSPC); + return NULL; } - unlock_return: file_unlock (file_fd); + file_offset = write_offset + sizeof (struct utmp); + pbuf = (struct utmp *) data; return pbuf; }