It is slightly confusing to allow adding these only to later refuse them.
Here is a (lightly tested :) patch to also refuse them when adding.
Signed-off-by: Tycho Andersen <tycho@tycho.pizza>
Check for close(2) failure at more places closing a file descriptor
written to.
Also ignore failures with errno set to EINTR (see man:close(2) for
details).
- 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>
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>
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>
If we consider simple objects as arrays of size 1, we can considerably
simplify these APIs, merging the *ARRAY and the non-array variants.
That will produce more readable code, since lines will be shorter (by
not having ARRAY in the macro names, as all macros will consistently
handle arrays), and the allocated size will be also more explicit.
The syntax will now be of the form:
p = MALLOC(42, foo_t); // allocate 42 elements of type foo_t.
p = MALLOC(1, bar_t); // allocate 1 element of type foo_t.
The _array() allocation functions should _never_ be called directly, and
instead these macros should be used.
The non-array functions (e.g., malloc(3)) still have their place, but
are limited to allocating structures with flexible array members. For
any other uses, the macros should be used.
Thus, we don't use any array or ARRAY variants in any code any more, and
they are only used as implementation details of these macros.
Link: <https://software.codidact.com/posts/285898/288023#answer-288023>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alloca(3) fails silently if not enough memory can be allocated on the
stack. Use checked dynamic allocation instead.
Also drop unnecessary manual NUL assignment, ensured by snprintf(3).
Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Allocate enough memory for the strings, two slashes and the NUL
terminator.
Reported-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
The --gid option accepts a group name or id. When a name is provided, it
is resolved to an id by looking up the name in the group database
(/etc/group).
The --prefix option overides the location of the passwd and group
databases. I suspect the --gid option was overlooked when wiring up the
--prefix option.
useradd --gid already respects --prefix; this change makes usermod
behave the same way.
Fixes: b6b2c756c9
Signed-off-by: Mike Gilbert <floppym@gentoo.org>
Use of these macros, apart from the benefits mentioned in the commit
that adds the macros, has some other good side effects:
- Consistency in getting the size of the object from sizeof(type),
instead of a mix of sizeof(type) sometimes and sizeof(*p) other
times.
- More readable code: no casts, and no sizeof(), so also shorter lines
that we don't need to cut.
- Consistency in using array allocation calls for allocations of arrays
of objects, even when the object size is 1.
Cc: Valentin V. Bartenev <vbartenev@gmail.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
- Every non-const pointer converts automatically to void *.
- Every pointer converts automatically to void *.
- void * converts to any other pointer.
- const void * converts to any other const pointer.
- Integer variables convert to each other.
I changed the declaration of a few variables in order to allow removing
a cast.
However, I didn't attempt to edit casts inside comparisons, since they
are very delicate. I also kept casts in variadic functions, since they
are necessary, and in allocation functions, because I have other plans
for them.
I also changed a few casts to int that are better as ptrdiff_t.
This change has triggered some warnings about const correctness issues,
which have also been fixed in this patch (see for example src/login.c).
Signed-off-by: Alejandro Colomar <alx@kernel.org>
In variadic functions we still do the cast. In POSIX, it's not
necessary, since NULL is required to be of type 'void *', and 'void *'
is guaranteed to have the same alignment and representation as 'char *'.
However, since ISO C still doesn't mandate that, and moreover they're
doing dubious stuff by adding nullptr, let's be on the cautious side.
Also, C++ requires that NULL is _not_ 'void *', but either plain 0 or
some magic stuff.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Report error if usermod asked for moving homedir and it does not exist.
Signed-off-by: Tomáš Mráz <tm@t8m.info>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
PARAMETERS:
According to the C2x charter, I reordered the parameters 'size'
and 'buf' from previously existing date_to_str() definitions.
C2x charter:
> 15. Application Programming Interfaces (APIs) should be
> self-documenting when possible. In particular, the order of
> parameters in function declarations should be arranged such that
> the size of an array appears before the array. The purpose is to
> allow Variable-Length Array (VLA) notation to be used. This not
> only makes the code's purpose clearer to human readers, but also
> makes static analysis easier. Any new APIs added to the Standard
> should take this into consideration.
I used 'long' for the date parameter, as some uses of the function
need to pass a negative value meaning "never".
FUNCTION BODY:
I didn't check '#ifdef HAVE_STRFTIME', which old definitions did,
since strftime(3) is guaranteed by the C89 standard, and all of
the conversion specifiers that we use are also specified by that
standard, so we don't need any extensions at all.
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
Alejandro Colomar
Skyler Ferrante
Tycho Andersen
Tobias Stoeckmann
Christian Göttsche
Dimitri John Ledkov
Iker Pedrosa
Martin Kletzander
Mike Gilbert
xyz
Jeremy Whiting
Matheus Marques
Serge Hallyn
Alejandro Colomar