With glibc we can use "e" in mode argument to set O_CLOEXEC on
opened files. The /etc/shadow and /etc/gshadow file handles should
be protected to make sure that they are never passed to child
processes by accident.
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Instead of reallocating 1 more meber per iteration, calculate the total
amount that we want by counting the number of commas (delimiters) in the
string, plus one for the last element, plus one for the terminating
NULL.
This might result in overallocation of one element if the string is an
empty string, or if there's a trailing comma; however, that's not an
issue. We can afford overallocating one element in certain cases, and
we get in exchange a much simpler function.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
We've simplified the function so much in the previous commits, that now
$2 is rather useless. It only sets the output parameter to the same
value that the function returns. It's simpler if the caller just sets
it itself after the call.
This removes the only 3-star pointer in the entire project. :)
Signed-off-by: Alejandro Colomar <alx@kernel.org>
0 is a horrible null-pointer constant. Don't use it.
Especially, when just a few lines above, in the same function,
we've used NULL for the same thing.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Use instead automatic variables as much as possible.
This reduces the number of dereferences, enhancing readability.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This makes build_list() less dependent on the context.
It starts from clean, whatever the state before the call was.
I was having a hard time understanding the reallocation,
until I saw that we were zeroing everything right before the call.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
If n was 0, it doesn't hurt to set it again to 0;
and the list would be NULL, so it doesn't hurt free(3)ing it
and setting to NULL again either.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It was hard to understand what each variable is. Use a consistent
scheme, where a 'p' means a pointer, 'l' means list, and 'n' means
number of elements. Those should be obvious from the name of the
function and the context, and will make it easier to read the code.
Also, the shorter names will allow focusing on the rest of the code.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
list ($2) is a pointer to a list of strings. We were declaring it as an
array of pointers to strings, which was bogus. It worked out of luck,
because array parameters are transformed into pointers by the compiler,
but it was incorrect. Just look at how we're calling this function.
$ grep build_list lib/gshadow.c
build_list(char *s, char ***list, size_t *nlist)
sgroup.sg_adm = build_list (fields[2], &admins, &nadmins);
sgroup.sg_mem = build_list (fields[3], &members, &nmembers);
$ grep '^static .*\<admins\>' lib/gshadow.c
static /*@null@*//*@only@*/char **admins = NULL;
$ grep '^static .*\<members\>' lib/gshadow.c
static /*@null@*//*@only@*/char **members = NULL;
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Except for the added (and sorted) includes, and the removal of redundant
parentheses, this patch can be approximated with the following semantic
patch:
$ cat ~/tmp/spatch/streq.sp;
@@
expression a, b;
@@
- strcmp(a, b) == 0
+ streq(a, b)
@@
expression a, b;
@@
- 0 == strcmp(a, b)
+ streq(a, b)
@@
expression a, b;
@@
- !strcmp(a, b)
+ streq(a, b)
$ find contrib/ lib* src/ -type f \
| xargs spatch --sp-file ~/tmp/spatch/streq.sp --in-place;
$ git restore lib/string/strcmp/streq.h;
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Before this patch, the function looped while (s != NULL && *s != '\0').
However, nothing was modifying that string if REALLOC() failed, so the
loop was forever.
Fixes: 8e167d28af ("[svn-upgrade] Integrating new upstream version, shadow (4.0.8)")
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>
* lib/gshadow.c (sgetsgent): Use strcpy(3) not strlcpy(3),
since the string is known to fit.
Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
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>
- Since strncpy(3) is not designed to write strings, but rather
(null-padded) character sequences (a.k.a. unterminated strings), we
had to manually append a '\0'. strlcpy(3) creates strings, so they
are always terminated. This removes dependencies between lines, and
also removes chances of accidents.
- Repurposing strncpy(3) to create strings requires calculating the
location of the terminating null byte, which involves a '-1'
calculation. This is a source of off-by-one bugs. The new code has
no '-1' calculations, so there's almost-zero chance of these bugs.
- strlcpy(3) doesn't padd with null bytes. Padding is relevant when
writing fixed-width buffers to binary files, when interfacing certain
APIs (I believe utmpx requires null padding at lease in some
systems), or when sending them to other processes or through the
network. This is not the case, so padding is effectively ignored.
- strlcpy(3) requires that the input string is really a string;
otherwise it crashes (SIGSEGV). Let's check if the input strings are
really strings:
- lib/fields.c:
- 'cp' was assigned from 'newft', and 'newft' comes from fgets(3).
- lib/gshadow.c:
- strlen(string) is calculated a few lines above.
- libmisc/console.c:
- 'cons' comes from getdef_str, which is a bit cryptic, but seems
to generate strings, I guess.1
- libmisc/date_to_str.c:
- It receives a string literal. :)
- libmisc/utmp.c:
- 'tname' comes from ttyname(3), which returns a string.
- src/su.c:
- 'tmp_name' has been passed to strcmp(3) a few lines above.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is Undefined Behavior to declare errno (see NOTES in its manual page).
Instead of using the errno dummy declaration, use one that doesn't need
a comment.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
* lib/gshadow.c: Remove __setsgNIS() -never used).
* lib/gshadow.c: Avoid multi-statements lines.
* lib/gshadow.c: Avoid assignments in comparisons.
* lib/gshadow.c: ptr[nelem] is a string. Initialize it to NULL
instead of '\0'.
* lib/gshadow.c: Add brackets and parenthesis.
* lib/gshadow.c: The size argument of strncpy is a size_t and the
size argument of fgets is an int.
Files with no license use the default 3-clauses BSD license. The copyright
were mostly not recorded; they were updated according to the Changelog.
"Julianne Frances Haugh and contributors" changed to "copyright holders
and contributors".
