Serge Hallyn [Wed, 4 Oct 2023 15:38:48 +0000 (10:38 -0500)]
Improve the login.defs unknown item error message
Closes #746
Only print the 'unknown item' message to syslog if we are
actually parsing a login.defs. Prefix it with "shadow:" to make
it clear in syslog where it came from.
Also add the source filename to the console message. I'm not
quite clear on the econf API, so not sure whether in that path we
will end up actually having the path, or printing ''.
This is not just a style issue. This should be a hard error, and never
compile. ISO C89 already had this feature as deprecated. ISO C99
removed this deprecated feature, for good reasons. If we compile
ignoring this warning, shadow is not going to behave well.
Cc: Sam James <sam@gentoo.org> Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/: Use asprintf(3) instead of strlen(3)+malloc(3)+snprintf(3)
asprintf(3) is non-standard, but is provided by GNU, the BSDs, and musl.
That makes it portable enough for us to use.
This function is much simpler than the burdensome code for allocating
the right size. Being simpler, it's thus safer.
I took the opportunity to fix the style to my preferred one in the
definitions of variables used in these calls, and also in the calls to
free(3) with these pointers. That isn't gratuituous, but has a reason:
it makes those appear in the diff for this patch, which helps review it.
Oh, well, I had an excuse :)
I remove this file with sadness, as it contains data from old times.
Unfortunately, this data is no longer relevant. The source code
management tool will keep it in memory.
Having source code in a compressed file doesn't seem like a good idea. I
checked several distributions and they don't distribute this binary, so
let's remove it.
Not sure what this file is exactly, but there's already a groupmems.c
that should generate the binary responsible for managing the members of
a user's primary group.
And do not set 'clear' to point to the empty string. After this commit,
'clear' only stores the result of getpass(3). This will be useful to
change the code to use agetpass().
There are no users of 'clear_pass' and 'wipe_clear_pass'.
$ grep -rn '\<clear_pass\>'
lib/pwauth.c:35:/*@null@*/char *clear_pass = NULL;
lib/pwauth.c:199: * not wipe it (the caller should wipe clear_pass when it is
lib/pwauth.c:203: clear_pass = clear;
$ grep -rn wipe_clear_pass
lib/pwauth.c:34:bool wipe_clear_pass = true;
lib/pwauth.c:198: * if the external variable wipe_clear_pass is zero, we will
lib/pwauth.c:204: if (wipe_clear_pass && (NULL != clear) && ('\0' != *clear)) {
ChangeLog:3813: * lib/pwauth.c: Use a boolean for wipe_clear_pass and use_skey.
zustr2stp.h: Assert some assumptions about the size
If the destination buffer is an array, we can check our assumptions.
This adds a readable way to explain that dsize must be strictly > ssize.
The reason is that the destination string is the source + '\0'.
If the destination is not an array, it's up to _FORTIFY_SOURCE or
-fanalyzer to catch newly introduced errors. There's nothing we can do;
at least not portably.
This function is like strlcpy(3), but returns -1 on truncation, which
makes it much easier to test. strlcpy(3) is useful in two cases:
- We don't care if the output is truncated. strlcpy(3) is fine for
those, and the return value can be ignored.
- Truncation is bad. In that case, we just want to signal truncation,
and the length of the original string is quite useless. Return the
length iff no truncation so that we can use it if necessary.
This simplifies the definition of the STRLCPY() macro.
It was blessed by POSIX.1-2001, and GCC says that it won't go away,
possibly ever.
memset(3) is dangerous, as the 2nd and 3rd arguments can be accidentally
swapped --who remembers what's the order of the 2nd and 3rd parameters
to memset(3) without checking the manual page or some code that uses
it?--. Some recent compilers may be able to catch that via some
warnings, but those are not infalible. And even if compiler warnings
could always catch that, the time lost in fixing or checking the docs is
lost for no clear gain. Having a sane API that is unambiguous is the
Right Thing (tm); and that API is bzero(3).
If someone doesn't believe memset(3) is error-prone, please read the
book "Unix Network Programming", Volume 1, 3rd Edition by Stevens, et
al., Section 1.2. See a stackoverflow reference in the link below[1].
bzero(3) had a bad fame in the bad old days, because some ancient
systems (I'm talking of many decades ago) shipped a broken version of
bzero(3). We can assume that all systems in which current shadow utils
can be built, have a working version of bzero(3) --if not, please fix
your broken system; don't blame the programmer--.
One reason that some use today to avoid bzero(3) in favor of memset(3)
is that memset(3) is more often used; but that's a circular reasoning.
Even if bzero(3) wasn't supported by the system, it would need to be
invented. It's the right API.
Another reason that some argue is that POSIX.1-2008 removed the
specification of bzero(3). That's not a problem, because GCC will
probably support it forever, and even if it didn't, we can redefine it
like we do with memzero(). bzero(3) is just a one-liner wrapper around
memset(3).
This makes it harder to make mistakes while editing the code. Since the
sizeof's can be autocalculated, let the machine do that. It also
reduces the cognitive load while reading the code.
It's not being used anymore. We got rid of it in favor of better APIs.
Well, it's still being used in one place: a contrib/ patch, but I
explicitly want to break it, so that someone reviews it. I don't want
to modify it, since it's not being tested, so it would be very risky for
me to touch it. Instead, let it bitrot, and if someone cares, they'll
update it correctly.
BTW, the comment that said /* danger -side effects */ was wrong:
sizeof() doesn't evaluate the argument (unless it's a VLA), so there
wasn't really a double-evaluation issue.
It wraps strlcpy(3bsd) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(dst) would be very bad).
- It calculates if there's truncation, returning an easy-to-use value.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
It's a wrapper around zustr2stp() that calls SIZEOF_ARRAY() internally.
The function call is usually --in our code base, always-- called with an
array as the second argument. For such an argument, one should call
SIZEOF_ARRAY(). To avoid mistakes, and simplify usage, let's add this
macro that does it internally.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then
the static_assert(3) within SIZEOF_ARRAY() makes sure VLAs are not
allowed).
These calls were intending to copy from a NUL-padded (possibly
non-NUL-terminated) character sequences contained in fixed-width arrays,
into a string, where extra padding is superfluous. Use the appropriate
call, which removes the superfluous work. That reduces the chance of
confusing maintainers about the intention of the code.
While at it, use the appropriate third parameter, which is the size of
the source buffer, and not the one of the destination buffer. As a side
effect, this reduces the use of '-1', which itself reduces the chance of
off-by-one bugs.
Also, since using sizeof() on an array is dangerous, use SIZEOF_ARRAY().
There's no standard function that copies from a null-padded character
sequence into a string.
A few standard functions can be workarounded to do that:
- strncat(3): This function is designed to catenate from a null-padded
character sequence into a string. The catch is that there's no
*cpy() equivalent of it --strncpy(3) is not at all related to
strncat(3); don't be fooled by the confusing name--, so one would
need to zero the first byte before the call to strncat(3). It also
has the inconvenient that it returns a useless value.
- strncpy(3): This function is designed to copy from a string to a
null-padded character sequence; the opposite of what we want to do.
If one passes the size of src instead of the size of dst, and then
manually zeroes the last byte of the dst buffer, something similar
to what we want happens. However, this does more than what we want:
it also padds with NUL the remaining bytes after the terminating NUL.
That extra work can confuse maintainers to believe that it's
necessary. That is exactly what happens in logout.c.
src/logoutd.c-46- /*
src/logoutd.c-47- * ut_user may not have the terminating NUL.
src/logoutd.c-48- */
src/logoutd.c:49: strncpy (user, ut->ut_user, sizeof (ut->ut_user));
src/logoutd.c-50- user[sizeof (ut->ut_user)] = '\0';
In that logout.c case --and in most invocations of strncpy(3), which
is usually a wrong tool-- the extra work is not wanted, so it's
preferrable to use the right tool, a function that does exactly
what's needed and nothing more than that. That tool is zustr2stp().
Read string_copying(7) for a more complete comparison of string copying
functions.
This makes it safe to call sizeof() on an array. Calling sizeof()
directly on an array is dangerous, because if the array changes to be a
pointer, the behavior will unexpectedly change. It's the same problem
as with NITEMS().
memzero.[ch]: Define memzero() and strzero() as inline functions
There's no need to have these as macros, so use functions, which are a
lot safer: there's no need to worry about multiple evaluation of args,
and there's also more type safety. Compiler warnings are also simpler,
as they don't dump all the nested macros.
These comments were wrong. Remove them instead of fixing them, since
now that we have this small header file, it's much easier to follow the
preprocessor conditionals.
lib, libmisc: Move source files to lib (where their headers were)
Scripted change:
$ find lib/ -type f \
| grep '\.h$' \
| sed 's,lib/,libmisc/,' \
| sed 's,\.h$,.c,' \
| xargs find 2>/dev/null \
| xargs mv -t lib/;
Plus updating the Makefiles.
Closes: <https://github.com/shadow-maint/shadow/issues/791> Closes: <https://bugs.gentoo.org/912446>
Link: <https://github.com/shadow-maint/shadow/issues/763#issuecomment-1664383425>
Link: <https://github.com/shadow-maint/shadow/pull/776>
Link: <https://github.com/shadow-maint/shadow/commit/d0518cc250afeaceb772a7f50a900cfc9b3ab937> Reported-by: Christian Bricart <christian@bricart.de> Reported-by: Robert Marmorstein <robert@marmorstein.org> Cc: Sam James <sam@gentoo.org>
[ jubalh tested the openSUSE package ] Tested-by: Michael Vetter <jubalh@iodoru.org> Acked-by: Michael Vetter <jubalh@iodoru.org>
[ Robert F. tested the Gentoo package ] Tested-by: Robert Förster <Dessa@gmake.de> Cc: David Seifert <soap@gentoo.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
sprintf(3) does not take the destination buffer into account. Although
the destination in these case is large enough, sprintf(3) indicates a
code smell.
commonio.c:522:15: warning: Although the value stored to 'cp' is used in the enclosing expression, the value is never actually read from 'cp' [deadcode.DeadStores]
write_full() doesn't write "up to" count bytes (which is write(2)'s
behavior, and exactly what this function is designed to avoid), but
rather exactly count bytes (on success).
- While fixing the documentation, take the time to add a man-page-like
comment as in other APIs. Especially, since we'll have to document
a few other changes from this patch, such as the modified return
values.
- Partial writes are still possible on error. It's the caller's
responsibility to handle that possibility.
API:
- In write(2), it's useful to know how many bytes were transferred,
since it can have short writes. In this API, since it either writes
it all or fails, that value is useless, and callers only want to know
if it succeeded or not. Thus, just return 0 or -1.
Implementation:
- Use `== -1` instead of `< 0` to check for write(2) syscall errors.
This is wisdom from Michael Kerrisk. This convention is useful
because it more explicitly tells maintainers that the only value
which can lead to that path is -1. Otherwise, a maintainer of the
code might be confused to think that other negative values are
possible. Keep it simple.
- The path under `if (res == 0)` was unreachable, since the loop
condition `while (count > 0)` precludes that possibility. Remove the
dead code.
- Use a temporary variable of type `const char *` to avoid a cast.
- Rename `res`, which just holds the result from write(2), to `w`,
which more clearly shows that it's just a very-short-lived variable
(by it's one-letter name), and also relates itself more to write(2).
I find it more readable.
- Move the definition of `w` to the top of the function. Now that the
function is significantly shorter, the lifetime of the variable is
clearer, and I find it more readable this way.
projects / thirdparty / shadow.git / log
