Message ID | 871rxof0by.fsf@oldenburg2.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | login: Fix updwtmp, updwtmx unlocking | expand |
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. */ >
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. */