Otherwise, the line is invalidly formatted, and we ignore it.
Detailed explanation:
There are two conditions on which we break out of the loops that precede
these added checks:
- j is too big (we've exhausted the space in the static arrays)
$ grep -r -e PORT_TTY -e PORT_IDS lib/port.*
lib/port.c: static char *ttys[PORT_TTY + 1]; /* some pointers to tty names */
lib/port.c: static char *users[PORT_IDS + 1]; /* some pointers to user ids */
lib/port.c: for (cp = buf, j = 0; j < PORT_TTY; j++) {
lib/port.c: if ((',' == *cp) && (j < PORT_IDS)) {
lib/port.h: * PORT_IDS - Allowable number of IDs per entry.
lib/port.h: * PORT_TTY - Allowable number of TTYs per entry.
lib/port.h:#define PORT_IDS 64
lib/port.h:#define PORT_TTY 64
- strpbrk(3) found a ':', which signals the end of the comma-sepatated
list, and the start of the next colon-separated field.
If the first character in the remainder of the string is not a ':', it
means we've exhausted the array size, but the CSV list was longer, so
we'd be truncating it. Consider the entire line invalid, and skip it.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: a19805445672 ("lib/port.c: getportent(): Make sure the aren't too many fields in the CSV")
Link: <https://github.com/shadow-maint/shadow/pull/1037>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Otherwise, the line is invalidly formatted, and we ignore it.
For fixing cherry-pick conflicts, this also includes (minimal) changes
from:
- 59e5eef38f89 ("contrib, lib/, src/, tests/: Use stpcpy(3) instead of its pattern")
<https://github.com/shadow-maint/shadow/pull/1035>
Closes: <https://github.com/shadow-maint/shadow/issues/1036>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: c3f97e251ef1 ("lib/port.c: getportent(): Make sure there are at least 2 ':' in the line")
Link: <https://github.com/shadow-maint/shadow/pull/1037>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
And do some style changes on the corresponding code.
For fixing cherry-pick conflicts, this also includes (minimal) changes
from:
- 964df6ed6ed6 ("lib/, src/: Use strchrnul(3) instead of its pattern")
<https://github.com/shadow-maint/shadow/pull/991>
- 59e5eef38f89 ("contrib, lib/, src/, tests/: Use stpcpy(3) instead of its pattern")
<https://github.com/shadow-maint/shadow/pull/1035>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: f1f82c21054b ("lib/port.c: getportent(): Remove obvious comments")
Link: <https://github.com/shadow-maint/shadow/pull/1037>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This label means we detected a bogus line, and want to skip it and jump
to the next one; rename it accordingly. 'again' seemed to say that it
was somehow looping on the same line.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: e790993c5d7c ("lib/port.c: getportent(): Rename goto label")
Link: <https://github.com/shadow-maint/shadow/pull/1037>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This was causing errors in my local testing in vms.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Cherry-picked-from: 2457fc7c6bc9 ("tests/run_some: make sure unshared root user can descend build dir")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
When we run for instance
check_subid_range ubuntu u 100000 65536
when ubuntu user is defined and has that range, it returns no entries
because the subid db is not opened. Open it in have_range if needed.
I haven't figured out why this ever worked.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Cherry-picked-from: 75ea679799a9 ("have_range: open the subid db if needed")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Fix a missing space after the -I path
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Cherry-picked-from: 81b5b2692501 ("libsubid test makefile: fix a typo")
Fixes: 6b9391b581fd ("tests/: Support run_some from exported tarball")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
volatile needs to be casted away behind a [[gnu::noipa]] function, to
make that invisible to the compiler. Otherwise, the compiler can see
that it is being discarded, and is free to abuse Undefined Behavior.
Closes: <https://github.com/shadow-maint/shadow/issues/1028>
Reported-by: Chris Hofstaedtler <zeha@debian.org>
Tested-by: Chris Hofstaedtler <zeha@debian.org>
Reviewed-by: Chris Hofstaedtler <zeha@debian.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 6e57238bf915 ("tests/unit/test_xasprintf.c: Fix use of volatile pointer")
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This is in preparation for the following commit.
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: 3307a8f4f0ac ("tests/unit/test_xasprintf.c: Cosmetic")
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Chris Hofstaedtler <zeha@debian.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
common/config.sh currently tries to find the top directory by looking
for .git. There are also many places under tests/ where we use
hard-coded ../../.. to find things like ${TOP_DIR}/lib.
We don't actually ship the tests with 'make dist'. So we will
be exporting tests/ as a separate tarball. In particular, I want
to then import this in the debian package. However, there it will
be under shadow.git/debian/tests, not shadow.git/tests.
To support this, accept the environment variable BUILD_BASE_DIR,
which should point to shadow.git.
An alternative would be to move the tests to their own git
tree. However, keeping tests in separate git tree tends to
lead to repos getting out of sync. And we'd still need to accept
something like BUILD_BASE_DIR.
Note there are a lot of tests under run-all, which I'm not converting
as they currently are not being run in CI, so I'm more likely to
break something.
Changelog:
2024 05 26: Incorporate feedback from alejandro-colomar
Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/21>
Link: <https://salsa.debian.org/debian/shadow/-/merge_requests/22>
Cc: Chris Hofstaedtler <zeha@debian.org>
Signed-off-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Cherry-picked-from: d55367bb161b ("tests/: Support run_some from exported tarball")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Groupmod -U may cause crashes because of double free. If without -a, the first free of (*ogrp).gr_mem is in gr_free_members(&grp), and then in gr_update without -n or gr_remove with -n.
Considering the minimal impact of modifications on existing code, delete gr_free_members(&grp) to avoid double free.Although this may seem reckless, the second free in two different positions will definitely be triggered, and the following two test cases can be used to illustrate the situation :
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -n g2 -U u1,u2 g1
Segmentation fault
This case would free (*ogrp).gr_mem in gr_free_members(&grp) due to assignment statements grp = *ogrp, then in if (nflg && (gr_remove (group_name) == 0)), which finally calls gr_free_members(grent) to free (*ogrp).gr_mem again.
[root@localhost src]# ./useradd u1
[root@localhost src]# ./useradd u2
[root@localhost src]# ./useradd u3
[root@localhost src]# ./groupadd -U u1,u2,u3 g1
[root@localhost src]# ./groupmod -U u1,u2 g1
Segmentation fault
The other case would free (*ogrp).gr_mem in gr_free_members(&grp) too, then in if (gr_update (&grp) == 0), which finally calls gr_free_members(grent) too to free (*ogrp).gr_mem again.
So the first free is unnecessary, maybe we can drop it.
Fixes: 342c934a3590 ("add -U option to groupadd and groupmod")
Closes: <https://github.com/shadow-maint/shadow/issues/1013>
Link: <https://github.com/shadow-maint/shadow/pull/1007>
Link: <https://github.com/shadow-maint/shadow/pull/271>
Link: <https://github.com/shadow-maint/shadow/issues/265>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Reviewed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: lixinyun <li.xinyun@h3c.com>
Per https://tdg.docbook.org/tdg/4.5/term, term is a word being
defined in a varlistentry. The 'high uid' description is not a
varlistentry, so <term> and </term> show up in the processed
manpage. See debian Bug#1072297.
Signed-off-by: Serge Hallyn <serge@hallyn.com>
`PKG_CONFIG` variable needs to be set for `PKG_CHECK_MODULES` to
succeed, but this wasn't happening in Fedora because the first
appearance of `PKG_CHECK_MODULES` was conditionally skipped because this
distribution is compiled without `libbsd` support. Thus, moving the
cmocka library detection before libbsd fixes the problem.
Suggested-by: Lukas Slebodnik <lslebodn@fedoraproject.org>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
All call sites have been replaced by functions from "atoi/a2i.h" and
"atoi/str2i.h" recently.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
It is a simpler call, with more type safety.
A consequence of this change is that the program now accepts numbers in
bases 8 and 16. That's not a problem here, I think.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
time_t isn't necessarily unsigned (in fact, it's likely to be signed.
Therefore, parse the number as the right type, via a2i(time_t, ...).
Still, reject negative numbers, just to be cautious. It was done
before (strtoull_noneg()), so it shouldn't be a problem. (However,
strtoull_noneg() was only introduced recently, and before that we called
strtoull(3), which silently accepted negative values.)
Remove the limitation of ULONG_MAX, which seems arbitrary. It probably
was written in times where 'time_t' had the same length of 'long', and
this was thus a test that the value didn't overflow 'time_t'. Such a
test is implicit in the a2i() call, so forget about it.
Unify the error messages into a single one that provides all the info
(except the value of 'fallback').
Link: <cb610d54b4 (r136407772)>
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Cc: Chris Lamb <lamby@debian.org>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Instead of raw sysconf(_SC_LOGIN_NAME_MAX) calls, which was being used
without error handling.
Fixes: 3b7cc053872c ("lib: replace `USER_NAME_MAX_LENGTH` macro")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Keep the while loop in the outer function, and move the iteration code
to this new helper. This makes it a bit more readable.
Cc: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
After _every_ iteration, 'changed' is always 'false'. We don't need to
have it outside of the loop.
See:
$ grepc update_gshadow_file . \
| grep -e changed -e goto -e continue -e break -e free_ngrp -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
bool changed;
changed = false;
while ((sgrp = sgr_next ()) != NULL) {
if (!was_member && !was_admin && !is_member) {
continue;
}
if (was_admin && lflg) {
changed = true;
}
if (was_member) {
if ((!Gflg) || is_member) {
if (lflg) {
changed = true;
}
} else {
changed = true;
}
} else if (is_member) {
changed = true;
}
if (!changed)
goto free_nsgrp;
changed = false;
}
}
This was already true in the commit that introduced the code:
$ git show 45c6603cc:src/usermod.c \
| grepc update_gshadow \
| grep -e changed -e goto -e break -e continue -e '\<if\>' -e '{' -e '}' \
| pcre2grep -v -M '{\n\t*}';
{
int changed;
changed = 0;
while ((sgrp = sgr_next())) {
* See if the user was a member of this group
* See if the user was an administrator of this group
* See if the user specified this group as one of their
if (!was_member && !was_admin && !is_member)
continue;
if (was_admin && lflg) {
changed = 1;
}
if (was_member && (!Gflg || is_member)) {
if (lflg) {
changed = 1;
}
} else if (was_member && Gflg && !is_member) {
changed = 1;
} else if (!was_member && Gflg && is_member) {
changed = 1;
}
if (!changed)
continue;
changed = 0;
}
}
Fixes: 45c6603cc86c ("[svn-upgrade] Integrating new upstream version, shadow (19990709)")
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This function creates a temporary file, and returns a FILE pointer to
it. This avoids dealing with both a file descriptor and a FILE pointer,
and correctly deallocating the resources on error.
The code before this patch was leaking the file descriptor if fdopen(3)
failed.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
See asprintf(3):
RETURN VALUE
When successful, these functions return the number of bytes
printed, just like sprintf(3). If memory allocation wasn’t possi‐
ble, or some other error occurs, these functions will return -1,
and the contents of strp are undefined.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
This will help add other labels in the following commits.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Resources should be freed in the inverse order of the allocation.
This refactor prepares for the following commits, which fix some leaks.
Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
