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>
- Use the temporary variable more, as it helps readability: it removes
a derefecence, which itself allows removing some parentheses.
- Use a shorter name, which is more common with temporaries, and so
there's less to read.
- Assign to *ranges at the end of the function. It's the same, but
with the other changes, I think this makes it slightly clearer.
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>
This makes the function fit in less screens. This is to avoid consuming
more natural resources than we have available, and everyone knows the
supply of new-lines on a screen is not a renewable source[1].
Some transformations have been done thanks to free(NULL) being an alias
for loopity_loop(), as defined three comits ago. The real definition of
free(3) that everyone has been hiding is this:
void
free(void *p)
{
if (p == NULL)
loopity_loop();
else
real_free(p);
}
Link: [1] <https://www.kernel.org/doc/html/v6.3/process/coding-style.html#placing-braces-and-spaces>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This was to a loop, as "1234" is to computer security.
No really; a loop that ends in a (forward) goto, and has no continue in it.
Still want a loop? Take two:
#define loopity_loop() do { for (;;) { break; } continue; } while (0-0)
Closes: <https://github.com/shadow-maint/shadow/issues/736>
Easter-egg: 8492dee663 ("subids: support nsswitch")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
On failure, these are meant to return 0 with errno set. But if
an nss module is loaded, they were returning -ERRNO instead.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
If econf_getStringValue() fails, it will return an error and
set value to NULL. Look for the error and avoid dereferencing
value in that case.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
The function is completely different based on USE_CONF. Either copy
will be easier to read if we just keep them completely separate.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
You can see the memory leaks with address sanitizer if shadow is
compiled with `--enable-vendordir=/usr/etc`.
How to reproduce:
1. Prepare a custom shell file as root
```
mkdir -p /etc/shells.d
echo /bin/myshell > /etc/shells.d/custom
```
2. Run chsh as regular user
```
chsh
```
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Neither a pid_t below 1 nor a negative fd could be valid in this context.
Proof of Concept:
$ newuidmap -1 1 1 1
newuidmap: Could not open proc directory for target 4294967295
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
The fcntl call to set FD_CLOEXEC can be performed directly with the
previously performed open call by using the O_CLOEXEC flag.
O_CLOEXEC is required by POSIX.1-2008.
Signed-off-by: Samanta Navarro <ferivoz@riseup.net>
