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.
lib/gshadow.c: build_list(): Remove second parameter
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. :)
lib/gshadow.c: sgetsgent(): Be consistent using NULL
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.
lib/gshadow.c: Move zeroing to within build_list()
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.
lib/gshadow.c: build_list(): Improve variable and parameter names
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.
lib/gshadow.c: build_list(): Fix type of parameter
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.
strsep(3) is stateless, and so is easier to reason about.
It also has a slight difference: strtok(3) jumps over empty fields,
while strsep(3) respects them as empty fields. In most of the cases
where we were using strtok(3), it makes more sense to respect empty
fields, and this commit probably silently fixes a few bugs.
In other cases (most notably filesystem paths), contiguous delimiters
("//") should be collapsed, so strtok(3) still makes more sense there.
This commit doesn't replace such strtok(3) calls.
While at this, remove some useless variables used by these calls, and
reduce the scope of others.
Except for the added (and sorted) includes, and the removal of redundant
parentheses, and one special case, this patch can be approximated with
the following semantic patch:
$ cat ~/tmp/spatch/strneq.sp;
@@
expression a, b;
@@
contrib/, lib/, src/: Use streq() instead of its pattern
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;
@@
The instructions are written so that this script should be run from the
root of the repository. Specify the path from the root of the repo.
Before this fix, the command needed to be run from within <share/>.
The get_map_ranges function shall support the whole accepted range
as specified in user_namespaces(7), i.e. upper and lower from 0 to
UINT_MAX - 1 as well as range from 1 to UINT_MAX. The actual limit of
range depends on values of upper and lower and adding the range
to either upper or lower shall never overflow UINT_MAX.
Fixes: 7c43eb2c4ea6 (2024-07-11, "lib/idmapping.c: get_map_ranges(): Move range check to a2ul() call") Fixes: ff2baed5dbf8 (2016-08-14, "idmapping: add more checks for overflow") Fixes: 94da3dc5c853 (2016-08-14, "also check upper for wrap") Fixes: 7f5a14817d30 (2016-07-31, "get_map_ranges: check for overflow") Co-authored-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/: Use NULL instead of 0 as a null pointer constant
GCC 15 will add -Wzero-as-null-pointer-constant for deprecating it,
and I'm working on a paper for deprecating it from ISO C too.
Let's remove any uses in our code base.
I've done this change by building GCC from master, adding
-Werror=zero-as-null-pointer-constant to ./autogen.sh, and fixing every
error that showed up.
/*
* Copy string to local buffer. It has to be tokenized and we
* have to do that to our private copy.
*/
- if (strlen (string) >= sizeof spwbuf) {
- fprintf (shadow_logfd,
- "%s: Too long passwd entry encountered, file corruption?\n",
- shadow_progname);
- return NULL; /* fail if too long */
- }
+ if (strlen (string) >= sizeof spwbuf)
+ return 0;
strcpy (spwbuf, string);
stpsep(spwbuf, "\n");
@@ -30,14 +26,16 @@
fields[i] = strsep(&cp, ":");
if (i == (FIELDS - 1))
- fields[i++] = "";
+ fields[i++] = empty;
if (cp != NULL || (i != FIELDS && i != OFIELDS))
- return NULL;
+ return 0;
/*
* Start populating the structure. The fields are all in
- * static storage, as is the structure we pass back.
+ * static storage, as is the structure we pass back. If we
+ * ever see a name with '+' as the first character, we try
+ * to turn on NIS processing.
*/
spwd.sp_namp = fields[0];
@@ -46,13 +44,13 @@
/*
* Get the last changed date. For all of the integer fields,
* we check for proper format. It is an error to have an
- * incorrectly formatted number.
+ * incorrectly formatted number, unless we are using NIS.
*/
/*
* Get the minimum period between password changes.
@@ -61,7 +59,7 @@
if (fields[3][0] == '\0')
spwd.sp_min = -1;
else if (a2sl(&spwd.sp_min, fields[3], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the maximum number of days a password is valid.
@@ -70,7 +68,7 @@
if (fields[4][0] == '\0')
spwd.sp_max = -1;
else if (a2sl(&spwd.sp_max, fields[4], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* If there are only OFIELDS fields (this is a SVR3.2 /etc/shadow
@@ -93,7 +91,7 @@
if (fields[5][0] == '\0')
spwd.sp_warn = -1;
else if (a2sl(&spwd.sp_warn, fields[5], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days of inactivity before an account is
@@ -103,7 +101,7 @@
if (fields[6][0] == '\0')
spwd.sp_inact = -1;
else if (a2sl(&spwd.sp_inact, fields[6], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* Get the number of days after the epoch before the account is
@@ -113,7 +111,7 @@
if (fields[7][0] == '\0')
spwd.sp_expire = -1;
else if (a2sl(&spwd.sp_expire, fields[7], NULL, 0, 0, LONG_MAX) == -1)
- return NULL;
+ return 0;
/*
* This field is reserved for future use. But it isn't supposed
@@ -123,8 +121,7 @@
if (fields[8][0] == '\0')
spwd.sp_flag = SHADOW_SP_FLAG_UNSET;
else if (str2ul(&spwd.sp_flag, fields[8]) == -1)
- return NULL;
+ return 0;
src/login_nopam.c: Rely on the system's MAXHOSTNAMELEN
The reason for that code seems to be some ancient AIX version that
defined a value that was too small (32). We don't support such systems.
In the link below, I found the following comment and code:
/*
* Some AIX versions advertise a too small MAXHOSTNAMELEN value (32).
* Result: long hostnames would be truncated, and connections would be
* dropped because of host name verification failures. Adrian van Bloois
* (A.vanBloois@info.nic.surfnet.nl) figured out what was the problem.
*/
This fix addresses an issue in is_valid_user_list() where the free
operation was attempted on an address not allocated with malloc(). By
duplicating the pointer with xstrdup(users) into dup, and using dup as
the original pointer, we ensure that only the valid pointer is freed,
avoiding an invalid free operation.
This bug was introduced when changing some code that used strchrnul(3)
to use strsep(3) instead. strsep(3) advances the pointer, unlike the
previous code.
This unconditionally leads to a bug:
- Passing NULL to free(3), if the last field in the
colon-separated-value list is non-empty. This results in a memory
leak.
- Passing a pointer to the null byte ('\0') that terminates the string,
if the last element of the colon-separated-value list is empty. The
most obvious reproducer of such a bogus free(3) call is:
free(strdup("foo:") + 4);
This results in Undefined Behavior, and could result in allocator
data corruption.
Fixes: 16cb66486554 (2024-07-01, "lib/, src/: Use strsep(3) instead of its pattern") Suggested-by: <https://github.com/frostb1ten> Reported-by: <https://github.com/frostb1ten> Reviewed-by: Serge Hallyn <serge@hallyn.com> Reviewed-by: Alejandro Colomar <alx@kernel.org> Cc: Iker Pedrosa <ipedrosa@redhat.com> Cc: Christian Brauner <christian@brauner.io>
Iker Pedrosa [Thu, 31 Oct 2024 08:25:57 +0000 (09:25 +0100)]
CI: fix fedora build problems
The new fedora 41 has been released and some things have changed. Make
sure to install python and python3-dnf and specify the dnf version in
the roles.
During coverity scan, there are reported four issues
with unbounded source buffer for each usage of input arg
directly with syslog function.
Sample coverity test report for chsh.c file:
1. string_size_argv: argv contains strings with unknown size.
int main (int argc, char **argv)
[...]
4. var_assign_var: Assigning: user = argv[optind]. Both are now tainted.
user = argv[optind];
[...]
CID 5771784: (#1 of 1): Unbounded source buffer (STRING_SIZE)
15. string_size: Passing string user of unknown size to syslog.
SYSLOG ((LOG_INFO, "changed user '%s' shell to '%s'", user, loginsh));
Similar issue is reported three times more:
File: chfn.c, function: main, variable: user
File: passwd.c, function: main, variable: name
File: newgrp.c, function: main, variable: group
This commit is the first approach to fix the reported issues.
The proposed changes add conditions, which verify
the user and group names arguments, including their lengths.
This will not silence the coverity reports, but the change causes
that they are irrelevant and could be ignored.
lib/alloc/realloc*.h: Always reallocate at least 1 byte
glibc's realloc(3) is broken. It was originally good (I believe) until
at some point, when it was changed to conform to C89, which had a bogus
specification that required that it returns NULL. C99 fixed the mistake
from C89, and so glibc's realloc(3) is non-conforming to
C99/C11/POSIX.1-2008. C17 broke again the definition of realloc(3).
Iker Pedrosa [Mon, 14 Oct 2024 09:53:50 +0000 (11:53 +0200)]
CI: update Ubuntu repositories configuration
Recently Ubuntu updated its repositories configuration file from
`/etc/apt/sources.list` to `/etc/apt/sources.list.d/ubuntu.source`.
Thus, we need to update its location to be able to install all the
package dependencies.
In addition, the CI script was trying to uncomment the lines starting
with `deb-src`, but there is none in the new configuration file format.
Replace `Types: deb` by `Types: deb deb-src` at the beginning of the
line instead.
This commit merges all dependency installation scripts into a single
workflow, which will be called from all sites that have to install
dependencies.
I forgot to remove the setting of errno when I switched from
strtoul_noneg() to str2ul(). strtoul(3) needs errno for determining
success, but str2ul() does not.
src/useradd.c: Add fmkomstemp() to fix mode of </etc/default/useradd>
The mode of the file should be 644, but mkstemp(2) was transforming it
to 600.
To do this, we need a function that accepts a mode parameter. While we
don't need a flags parameter, to avoid confusion with mkostemp(2), let's
add both a flags and a mode parameter.
The tz function is only called if ENV_TZ starts with a slash.
If the specified file cannot be read, the code implies that ENV_TZ
would be returned if it does not start with a slash.
Since we know that it DOES start with a slash, the code can be
simplified to state that "TZ=CST6CDT" is returned as a default if
the specified file cannot be read.
Benefit of this change is that strcpy's use case here can be
easier verified.
Since:
- utmpx APIs are used in non-Linux code blocks
- <utmpx.h> is already unconditionally included in Linux parts in other
files
then unconditionally include it in this file as well.
projects / thirdparty / shadow.git / log
