su.c:678:26: warning: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘const void *’ [-Wformat=]
su.c:681:44: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
su.c:683:46: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
Reported-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We don't need to terminate them manually after the call. Remove all
that paranoid code, which in some cases was even wrong. While at it,
let's do a few more things:
- Use sizeof(buf) for the size of the buffer. I found that a few cases
were passing one less byte (probably because the last one was
manually zeroed later). This caused a double NUL. snprintf(3) wants
the size of the entire buffer to properly terminate it. Passing the
exact value hardcoded is brittle, so use sizeof().
- Align and improve style of variable declarations. This makes them
appear in this diff, which will help review the patch.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Set errno = 0 before the call. Otherwise, it may contain anything.
- ERANGE is not the only possible errno value of these functions. They
can also set it to EINVAL.
- Any errno value after these calls is bad; just compare against 0.
- Don't check for the return value; just errno. This function is
guaranteed to not modify errno on success (POSIX).
- Check endptr == str, which may or may not set EINVAL.
Suggested-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Since failure() is [[noreturn]], we can invert the conditional so that
we don't need an else. This silences a -Wunused-parameter warning.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This simplifies the code a little bit, and prepares for the next
commits, which will clean up further.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Upcoming `gcc-14` enabled a few warnings into errors, like
`-Wimplicit-function-declaration`. This caused `shadow` build to fail
as:
pwunconv.c: In function 'main':
pwunconv.c:132:13: error: implicit declaration of function 'getdef_bool' [-Wimplicit-function-declaration]
132 | if (getdef_bool("USE_TCB")) {
| ^~~~~~~~~~~
The change adds missing include headers.
If an entry in /etc/shells is not an absolute path (comments or
partial reads due to fgets), the line should not be considered as
a valid login shell.
In general all systems should have getusershells, but let's better
be safe than sorry.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
These functions don't seem to exist anymore. I can't find them in
Debian, nor in a web search. They probably were functions from an
ancient implementation of cracklib that doesn't exist anymore.
$ git remote -v
origin git@github.com:cracklib/cracklib.git (fetch)
origin git@github.com:cracklib/cracklib.git (push)
$ grep -rni fascisthistory
$ git log --grep FascistHistory
$ git log -S FascistHistory
Closes: <https://codesearch.debian.net/search?q=FascistHistory&literal=1>
Cc: Mike Frysinger <vapier@gentoo.org>
Acked-by: Michael Vetter <jubalh@iodoru.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
ut_line doesn't hold a string. It is a null-padded fixed-width array.
Luckily, I don't think there has ever existed a ut_line ("/dev/tty*")
that was 32 bytes long. That would have resulted in a buffer overrun.
Anyway, do the right thing, which is copying into a temporary string.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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 ''.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
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 :)
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Badnames still accepted, note that previously usage already stated
singular form, whilst manpage and real one was plural only.
Fixes: 45d6746219 ("src: correct "badname" option")
Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com>
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).
Link: [1] <https://stackoverflow.com/a/17097978>
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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().
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Documentation:
- Correct the comment documenting the function:
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.
Use:
- Also use `== -1` to check errors.
Cc: Christian Göttsche <cgzones@googlemail.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
With the recent changes both login and su compilation fail because there
are some missing dependencies from SELINUX library. Thus, add LIBSELINUX
to su and login for those cases where the library is used.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
