From patchwork Mon Aug 12 09:56:03 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 34054 Received: (qmail 94675 invoked by alias); 12 Aug 2019 09:56:09 -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 94661 invoked by uid 89); 12 Aug 2019 09:56:09 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=5272, user's, cancel, handler X-HELO: mx1.redhat.com From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH] login: Replace macro-based control flow with function calls in utmp Date: Mon, 12 Aug 2019 11:56:03 +0200 Message-ID: <87ef1qvhak.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 2019-08-05 Florian Weimer * login/utmp_file.c (LOCK_FILE, LOCKING_FAILED, UNLOCK_FILE): Remove macros. (struct file_locking): New. (file_locking_failed, file_locking_unlock, file_locking_restore): New function. (__libc_getutent_r): Use the new functions. (internal_getut_r): Likewise. (__libc_getutline_r): Likewise. (__libc_pututline): Likewise. (__libc_updwtmp): Likewise. Reviewed-by: Adhemerval Zanella diff --git a/login/utmp_file.c b/login/utmp_file.c index 9badf11fb3..3f21de2b71 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -52,58 +52,72 @@ static struct utmp last_entry; /* Do-nothing handler for locking timeout. */ static void timeout_handler (int signum) {}; -/* LOCK_FILE(fd, type) failure_statement - attempts to get a lock on the utmp file referenced by FD. If it fails, - the failure_statement is executed, otherwise it is skipped. - LOCKING_FAILED() - jumps into the UNLOCK_FILE macro and ensures cleanup of LOCK_FILE. - UNLOCK_FILE(fd) - unlocks the utmp file referenced by FD and performs the cleanup of - LOCK_FILE. - */ -#define LOCK_FILE(fd, type) \ -{ \ - struct flock fl; \ - struct sigaction action, old_action; \ - unsigned int old_timeout; \ - \ - /* Cancel any existing alarm. */ \ - old_timeout = alarm (0); \ - \ - /* Establish signal handler. */ \ - action.sa_handler = timeout_handler; \ - __sigemptyset (&action.sa_mask); \ - action.sa_flags = 0; \ - __sigaction (SIGALRM, &action, &old_action); \ - \ - alarm (TIMEOUT); \ - \ - /* Try to get the lock. */ \ - memset (&fl, '\0', sizeof (struct flock)); \ - fl.l_type = (type); \ - fl.l_whence = SEEK_SET; \ - if (__fcntl64_nocancel ((fd), F_SETLKW, &fl) < 0) - -#define LOCKING_FAILED() \ - goto unalarm_return - -#define UNLOCK_FILE(fd) \ - /* Unlock the file. */ \ - fl.l_type = F_UNLCK; \ - __fcntl64_nocancel ((fd), F_SETLKW, &fl); \ - \ - unalarm_return: \ - /* 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); \ -} while (0) + + +/* 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; +}; + +static bool +file_locking_failed (struct file_locking *locking, int fd, int type) +{ + /* Cancel any existing alarm. */ + locking->old_timeout = alarm (0); + + /* Establish signal handler. */ + struct sigaction action; + action.sa_handler = timeout_handler; + __sigemptyset (&action.sa_mask); + action.sa_flags = 0; + __sigaction (SIGALRM, &action, &locking->old_action); + + alarm (TIMEOUT); + + /* Try to get the lock. */ + struct flock fl = + { + .l_type = type, + fl.l_whence = SEEK_SET, + }; + return __fcntl64_nocancel (fd, F_SETLKW, &fl) < 0; +} + +static void +file_locking_unlock (int fd) +{ + struct flock fl = + { + .l_type = F_UNLCK, + }; + __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) @@ -153,16 +167,16 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result) return -1; } - LOCK_FILE (file_fd, F_RDLCK) + struct file_locking fl; + if (file_locking_failed (&fl, file_fd, F_RDLCK)) + nbytes = 0; + else { - nbytes = 0; - LOCKING_FAILED (); + /* Read the next entry. */ + nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp)); + file_locking_unlock (file_fd); } - - /* Read the next entry. */ - nbytes = __read_nocancel (file_fd, &last_entry, sizeof (struct utmp)); - - UNLOCK_FILE (file_fd); + file_locking_restore (&fl); if (nbytes != sizeof (struct utmp)) { @@ -188,10 +202,12 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer, { int result = -1; - LOCK_FILE (file_fd, F_RDLCK) + struct file_locking fl; + if (file_locking_failed (&fl, file_fd, F_RDLCK)) { *lock_failed = true; - LOCKING_FAILED (); + file_locking_restore (&fl); + return -1; } if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME @@ -241,7 +257,8 @@ internal_getut_r (const struct utmp *id, struct utmp *buffer, result = 0; unlock_return: - UNLOCK_FILE (file_fd); + file_locking_unlock (file_fd); + file_locking_restore (&fl); return result; } @@ -287,10 +304,12 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer, return -1; } - LOCK_FILE (file_fd, F_RDLCK) + struct file_locking fl; + if (file_locking_failed (&fl, file_fd, F_RDLCK)) { *result = NULL; - LOCKING_FAILED (); + file_locking_restore (&fl); + return -1; } while (1) @@ -318,7 +337,8 @@ __libc_getutline_r (const struct utmp *line, struct utmp *buffer, *result = buffer; unlock_return: - UNLOCK_FILE (file_fd); + file_locking_unlock (file_fd); + file_locking_restore (&fl); return ((*result == NULL) ? -1 : 0); } @@ -375,10 +395,11 @@ __libc_pututline (const struct utmp *data) } } - LOCK_FILE (file_fd, F_WRLCK) + struct file_locking fl; + if (file_locking_failed (&fl, file_fd, F_WRLCK)) { - pbuf = NULL; - LOCKING_FAILED (); + file_locking_restore (&fl); + return NULL; } if (found < 0) @@ -421,7 +442,8 @@ __libc_pututline (const struct utmp *data) } unlock_return: - UNLOCK_FILE (file_fd); + file_locking_unlock (file_fd); + file_locking_restore (&fl); return pbuf; } @@ -450,8 +472,13 @@ __libc_updwtmp (const char *file, const struct utmp *utmp) if (fd < 0) return -1; - LOCK_FILE (fd, F_WRLCK) - LOCKING_FAILED (); + struct file_locking fl; + if (file_locking_failed (&fl, fd, F_WRLCK)) + { + file_locking_restore (&fl); + __close_nocancel_nostatus (fd); + return -1; + } /* Remember original size of log file. */ offset = __lseek64 (fd, 0, SEEK_END); @@ -477,7 +504,8 @@ __libc_updwtmp (const char *file, const struct utmp *utmp) result = 0; unlock_return: - UNLOCK_FILE (fd); + file_locking_unlock (file_fd); + file_locking_restore (&fl); /* Close WTMP file. */ __close_nocancel_nostatus (fd);