* [PATCH] login: Fix updwtmp, updwtmx unlocking
@ 2019-08-14 11:33 Florian Weimer
2019-08-14 20:10 ` Carlos O'Donell
0 siblings, 1 reply; 2+ messages in thread
From: Florian Weimer @ 2019-08-14 11:33 UTC (permalink / raw)
To: libc-alpha
Commit 5a3afa9738f3dbbaf8c0a35665318c1af782111b (login: Replace
macro-based control flow with function calls in utmp) introduced
a regression because after it, __libc_updwtmp attempts to unlock
the wrong file descriptor.
2019-08-14 Florian Weimer <fweimer@redhat.com>
* login/utmp_file.c (__libc_updwtmp): Unlock the right file
descriptor.
* login/Makefile (tests): Add tst-updwtmpx.
* login/tst-updwtmpx.c: New file.
diff --git a/login/Makefile b/login/Makefile
index 92535f0aec..f9c4264087 100644
--- a/login/Makefile
+++ b/login/Makefile
@@ -43,7 +43,7 @@ endif
subdir-dirs = programs
vpath %.c programs
-tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin
+tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx
# Build the -lutil library with these extra functions.
extra-libs := libutil
diff --git a/login/tst-updwtmpx.c b/login/tst-updwtmpx.c
new file mode 100644
index 0000000000..4eb4a3fbcd
--- /dev/null
+++ b/login/tst-updwtmpx.c
@@ -0,0 +1,108 @@
+/* Basic test coverage for updwtmpx.
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; see the file COPYING.LIB. If
+ not, see <http://www.gnu.org/licenses/>. */
+
+#include <errno.h>
+#include <stdlib.h>
+#include <support/check.h>
+#include <support/descriptors.h>
+#include <support/support.h>
+#include <support/temp_file.h>
+#include <support/test-driver.h>
+#include <support/xunistd.h>
+#include <unistd.h>
+#include <utmpx.h>
+
+static int
+do_test (void)
+{
+ /* Two entries filled with an arbitrary bit pattern. */
+ struct utmpx entries[2];
+ unsigned char pad;
+ {
+ unsigned char *p = (unsigned char *) &entries[0];
+ for (size_t i = 0; i < sizeof (entries); ++i)
+ {
+ p[i] = i;
+ }
+ /* Make sure that the first and second entry and the padding are
+ different. */
+ p[sizeof (struct utmpx)] = p[0] + 1;
+ pad = p[0] + 2;
+ }
+
+ char *path;
+ int fd = create_temp_file ("tst-updwtmpx-", &path);
+
+ /* Used to check that updwtmpx does not leave an open file
+ descriptor around. */
+ struct support_descriptors *descriptors = support_descriptors_list ();
+
+ /* updwtmpx is expected to remove misalignment. Optionally insert
+ one byte of misalignment at the start and in the middle (after
+ the first entry). */
+ for (int misaligned_start = 0; misaligned_start < 2; ++misaligned_start)
+ for (int misaligned_middle = 0; misaligned_middle < 2; ++misaligned_middle)
+ {
+ if (test_verbose > 0)
+ printf ("info: misaligned_start=%d misaligned_middle=%d\n",
+ misaligned_start, misaligned_middle);
+
+ xftruncate (fd, 0);
+ TEST_COMPARE (pwrite64 (fd, &pad, misaligned_start, 0),
+ misaligned_start);
+
+ /* Write first entry and check it. */
+ errno = 0;
+ updwtmpx (path, &entries[0]);
+ TEST_COMPARE (errno, 0);
+ support_descriptors_check (descriptors);
+ TEST_COMPARE (xlseek (fd, 0, SEEK_END), sizeof (struct utmpx));
+ struct utmpx buffer;
+ TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
+ sizeof (buffer));
+ TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
+ &buffer, sizeof (buffer));
+
+ /* Middle mis-alignmet. */
+ TEST_COMPARE (pwrite64 (fd, &pad, misaligned_middle,
+ sizeof (struct utmpx)), misaligned_middle);
+
+ /* Write second entry and check both entries. */
+ errno = 0;
+ updwtmpx (path, &entries[1]);
+ TEST_COMPARE (errno, 0);
+ support_descriptors_check (descriptors);
+ TEST_COMPARE (xlseek (fd, 0, SEEK_END), 2 * sizeof (struct utmpx));
+ TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
+ sizeof (buffer));
+ TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
+ &buffer, sizeof (buffer));
+ TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), sizeof (buffer)),
+ sizeof (buffer));
+ TEST_COMPARE_BLOB (&entries[1], sizeof (entries[1]),
+ &buffer, sizeof (buffer));
+ }
+
+ support_descriptors_free (descriptors);
+ free (path);
+ xclose (fd);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/login/utmp_file.c b/login/utmp_file.c
index 7bd6034af4..5e4e66d1d0 100644
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -503,7 +503,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
result = 0;
unlock_return:
- file_unlock (file_fd);
+ file_unlock (fd);
file_lock_restore (&fl);
/* Close WTMP file. */
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] login: Fix updwtmp, updwtmx unlocking
2019-08-14 11:33 [PATCH] login: Fix updwtmp, updwtmx unlocking Florian Weimer
@ 2019-08-14 20:10 ` Carlos O'Donell
0 siblings, 0 replies; 2+ messages in thread
From: Carlos O'Donell @ 2019-08-14 20:10 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
On 8/14/19 7:33 AM, Florian Weimer wrote:
> Commit 5a3afa9738f3dbbaf8c0a35665318c1af782111b (login: Replace
> macro-based control flow with function calls in utmp) introduced
> a regression because after it, __libc_updwtmp attempts to unlock
> the wrong file descriptor.
This makes sense, and it was an easy mistake to make because many
other functions use the same pattern to unlock file_fd.
OK for master if you add a block comment describing the test to the
testsuite.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 2019-08-14 Florian Weimer <fweimer@redhat.com>
>
> * login/utmp_file.c (__libc_updwtmp): Unlock the right file
> descriptor.
> * login/Makefile (tests): Add tst-updwtmpx.
> * login/tst-updwtmpx.c: New file.
>
> diff --git a/login/Makefile b/login/Makefile
> index 92535f0aec..f9c4264087 100644
> --- a/login/Makefile
> +++ b/login/Makefile
> @@ -43,7 +43,7 @@ endif
> subdir-dirs = programs
> vpath %.c programs
>
> -tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin
> +tests := tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-updwtmpx
OK.
>
> # Build the -lutil library with these extra functions.
> extra-libs := libutil
> diff --git a/login/tst-updwtmpx.c b/login/tst-updwtmpx.c
> new file mode 100644
> index 0000000000..4eb4a3fbcd
> --- /dev/null
> +++ b/login/tst-updwtmpx.c
> @@ -0,0 +1,108 @@
> +/* Basic test coverage for updwtmpx.
OK. Because we broke __libc_updwtmp.
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public License as
> + published by the Free Software Foundation; either version 2.1 of the
> + License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; see the file COPYING.LIB. If
> + not, see <http://www.gnu.org/licenses/>. */
> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/descriptors.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/test-driver.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +#include <utmpx.h>
> +
Needs a one paragraph description of the intent of the test.
> +static int
> +do_test (void)
> +{
> + /* Two entries filled with an arbitrary bit pattern. */
> + struct utmpx entries[2];
OK.
> + unsigned char pad;
> + {
> + unsigned char *p = (unsigned char *) &entries[0];
> + for (size_t i = 0; i < sizeof (entries); ++i)
> + {
> + p[i] = i;
> + }
> + /* Make sure that the first and second entry and the padding are
> + different. */
> + p[sizeof (struct utmpx)] = p[0] + 1;
> + pad = p[0] + 2;
> + }
OK.
> +
> + char *path;
> + int fd = create_temp_file ("tst-updwtmpx-", &path);
OK.
> +
> + /* Used to check that updwtmpx does not leave an open file
> + descriptor around. */
> + struct support_descriptors *descriptors = support_descriptors_list ();
OK.
> +
> + /* updwtmpx is expected to remove misalignment. Optionally insert
> + one byte of misalignment at the start and in the middle (after
> + the first entry). */
OK.
> + for (int misaligned_start = 0; misaligned_start < 2; ++misaligned_start)
> + for (int misaligned_middle = 0; misaligned_middle < 2; ++misaligned_middle)
> + {
> + if (test_verbose > 0)
> + printf ("info: misaligned_start=%d misaligned_middle=%d\n",
> + misaligned_start, misaligned_middle);
> +
> + xftruncate (fd, 0);
> + TEST_COMPARE (pwrite64 (fd, &pad, misaligned_start, 0),
> + misaligned_start);
> +
> + /* Write first entry and check it. */
> + errno = 0;
> + updwtmpx (path, &entries[0]);
> + TEST_COMPARE (errno, 0);
> + support_descriptors_check (descriptors);
> + TEST_COMPARE (xlseek (fd, 0, SEEK_END), sizeof (struct utmpx));
> + struct utmpx buffer;
> + TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
> + sizeof (buffer));
> + TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
> + &buffer, sizeof (buffer));
OK.
> +
> + /* Middle mis-alignmet. */
> + TEST_COMPARE (pwrite64 (fd, &pad, misaligned_middle,
> + sizeof (struct utmpx)), misaligned_middle);
> +
> + /* Write second entry and check both entries. */
> + errno = 0;
> + updwtmpx (path, &entries[1]);
> + TEST_COMPARE (errno, 0);
> + support_descriptors_check (descriptors);
> + TEST_COMPARE (xlseek (fd, 0, SEEK_END), 2 * sizeof (struct utmpx));
> + TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), 0),
> + sizeof (buffer));
> + TEST_COMPARE_BLOB (&entries[0], sizeof (entries[0]),
> + &buffer, sizeof (buffer));
> + TEST_COMPARE (pread64 (fd, &buffer, sizeof (buffer), sizeof (buffer)),
> + sizeof (buffer));
> + TEST_COMPARE_BLOB (&entries[1], sizeof (entries[1]),
> + &buffer, sizeof (buffer));
OK.
> + }
> +
> + support_descriptors_free (descriptors);
> + free (path);
> + xclose (fd);
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/login/utmp_file.c b/login/utmp_file.c
> index 7bd6034af4..5e4e66d1d0 100644
> --- a/login/utmp_file.c
> +++ b/login/utmp_file.c
> @@ -503,7 +503,7 @@ __libc_updwtmp (const char *file, const struct utmp *utmp)
> result = 0;
>
> unlock_return:
> - file_unlock (file_fd);
> + file_unlock (fd);
OK. Reviewed the surrounding code, and verified that __libc_updwtmp uses 'fd' because
it's what was locked earlier in the function, and that file_fd should not be used
(which is only for setutent/getutent/endutent/getutline... and others which don't
open their own file).
> file_lock_restore (&fl);
>
> /* Close WTMP file. */
>
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-08-14 20:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 11:33 [PATCH] login: Fix updwtmp, updwtmx unlocking Florian Weimer
2019-08-14 20:10 ` Carlos O'Donell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).