strftime(3) makes no guarantees about the contents of the buffer if the
formatted string wouldn't fit in the buffer. It simply returns 0, and
it's the programmer's responsibility to do the right thing after that.
Let's write the string "future" if there's an error, similar to what we
do with gmtime(3)'s errors.
Also, `buf[size - 1] = '\0';` didn't make sense. If the copy fits,
strftime(3) guarantees to terminate with NUL. If it doesn't, the entire
contents of buf are undefined, so adding a NUL at the end of the buffer
would be dangerous: the string could contain anything, such as
"gimme root access now". Remove that, now that we set the string to
"future", as with gmtime(3) errors. This setting to '\0' comes from the
times when we used strncpy(3) in the implementation, and should have
been removed when I changed it to use strlcpy(3); however, I didn't
check we didn't need it anymore.
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
There's been a very long and interesting discussion in linux-man@ and
libc-alpha@, where we've discussed all the string-copying functions,
their pros and cons, when should each be used and avoided, etc.
Paul Eggert pointed out an important problem of strlcpy(3): it is
vulnerable to DoS attacks if an attacker controls the length of the
source string. And even if it doesn't control it, the function is dead
slow (because its API forces it to calculate strlen(src)).
We've agreed that the general solution for a truncating string-copying
function is to write a wrapper over strnlen(3)+memcpy(3), which is
limited to strnlen(src, sizeof(dst)). This is not vulnerable to DoS,
and is very fast for all buffer sizes. string_copying(7) has been
updated to reflect this, and provides a reference implementation for
this wrapper function.
This strtcpy(3) (t for truncation) wrapper happens to have the same API
that our strlcpy_() function had, so replace it with the better
implementation. We don't need to update callers nor tests, since the
API is the same.
A future commit will rename STRLCPY() to STRTCPY(), and replace
remaining calls to strlcpy(3) by calls to this strtcpy(3).
Link: <https://lore.kernel.org/linux-man/ZU4SDh-Se5gjPny5@debian/T/#mfb5a3fdeb35487dec6f8d9e3d8548bd0d92c4975/>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
free(NULL) is valid; there's no need to check for NULL. Simplify.
Fixes: 5178f8c5af ("utmp: call prepare_utmp() even if utent is NULL")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
copydir.c:429:4: warning: Value stored to 'err' is never read [deadcode.DeadStores]
Also reduce indentation by bailing out early.
(cherry picked from commit d89f2fb06d1b81b56299f9d0bfe7a927a2282f19)
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>
As other x...() wrappers around functions that allocate, these wrappers
are like [v]asprintf(3), but exit on failure.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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().
$ grep '\<clear\>' lib/pwauth.c;
char *clear = NULL;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
clear = getpass (prompt);
input = (clear == NULL) ? "" : clear;
if (NULL != clear) {
strzero (clear);
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.
Remove them.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.
Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
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>
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.
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>
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).
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>
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).
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>
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.
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 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().
Link: <https://stackoverflow.com/a/57537491>
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>
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.
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 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.
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>
