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>
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.
Use snprintf(3).
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]
Reviewed-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>
Remove `utmp` structure as an argument and include its logic inside the
function. This will help remove any reference to utmp from login.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
The functionality from this function is related to utmp. Restrict access
to `setutmp()` to the same file.
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
