Assorted static analyzer inspired fixes #639

cgzones · 2023-01-30T16:46:36Z

No description provided.

alejandro-colomar · 2023-01-30T22:35:33Z

Drop unnecessary cast to same type:
useradd: free string:
loginprompt: do not use exit in signal handler:
lib/btrfs: avoid NULL-dereference:
login: use strlcpy to always NULL terminate (but see comment about commit message and NULL):
usermod/groups: use checked malloc:
Reviewed-by: Alejandro Colomar <alx@kernel.org>

libmisc/copydir: do not forget errors from directory copy:
chgpasswd: avoid NULL dereference:
I don't really understand what the code is doing enough to say I reviewed it, but the changes look reasonable.

src/chage.c

alejandro-colomar · 2023-01-30T23:05:51Z

lib/commonio.c

-		while (   ((cp = strrchr (buf, '\n')) == NULL)
+		while (   (strrchr (buf, '\n') == NULL)


This whole loop seems like some getline(3) implementation. If so, we could just call it. I'll check and send a patch for it.

It's still a fast draft, and needs some careful review, but it's here:

#640

libmisc/failure.c

alejandro-colomar · 2023-01-30T23:59:36Z

lib/commonio.c

+		r = snprintf (buf, sizeof buf, "%s-", db->filename);
+		if (r < 0 || (size_t)r >= sizeof buf) {
+			(void) fclose (db->fp);
+			db->fp = NULL;


I don't see this NULL being used, so I guess it's just for preventing use-after-free errors. I disagree with that approach. If you don't null the pointer, powerful static analyzers may detect use-after-free errors. However, if you null it, static analyzers will consider that you intend to use that NULL value as a valid pointer (to nowhere), and not warn about such errors, which are still programming errors.

I suggest removing that line, and letting things like GCC's -fanalyzer catch such bugs.

On the contrary: Given most memory is allocated uninitialized with malloc throughout the code base (bug in itself, but well), setting things to NULL actively prevents use-after-free issues (cleanly detectable runtime crash if not cought in the code). Which brings us to the second point: modern compilers allow to mark functions as requiring pointers to be non-NULL, thus forcing you to either check that assumption or pass it on in your library signature. This is very powerful, in itself and can find loads of memory issues even at compile-time. That aside, uninitialized memory is subject to ASAN, which together with LSAN can uncover many memory issues before they become critical bugs.

Or in short: While this line seems unnecessary there's a good reason to keep it, which is called defensive programming, which is the foundation for the even more powerful tool called defense-in-depth …

On the contrary: Given most memory is allocated uninitialized with malloc throughout the code base (bug in itself, but well), setting things to NULL actively prevents use-after-free issues (cleanly detectable runtime crash if not cought in the code). Which brings us to the second point: modern compilers allow to mark functions as requiring pointers to be non-NULL, thus forcing you to either check that assumption or pass it on in your library signature.

There are two such things:

[[gnu::nonnull]]: This one is quite dreaded by programmers because it only allows the compiler to notice more places with Undefined Behavior and optimize accordingly with few warnings.

Clang's _Nonnull: This one is only supported by Clang. It doesn't allow the compiler to take advantage of Undefined Behavior, and requires it to warn, which is a good thing. However, Clang's warnings about it are quite bad, and I'd turn them off. See [-Wnullable-to-nonnull-conversion] should warn in more cases, and shouldn't in some llvm/llvm-project#57546. While I'm promoting this feature through the man-pages, and I hope it gets improved in the future, I don't think it's mature yet to rely on it.

I've heard there's something a bit better than Clang about _Nonnull (and the better qualifier _Nullable), which is Clang Static Analyzer. However, it's a pretty bad design that only allows you to run though a black box which you must trust. I can't run that like all other linters I use, by just having a command that I run instead of the compiler, and I can fine tune. Instead, I must run it instead of my build system, and assume it will know what to do. That's instane. In fact, if I search for their documentation, the first thing I see is a screenshot of how to use it in macOS. If a GUI is the main interface which it was designed to be used with, it's definitely not something I'll look into.

This is very powerful, in itself and can find loads of memory issues even at compile-time. That aside, uninitialized memory is subject to ASAN, which together with LSAN can uncover many memory issues before they become critical bugs.

On the contrary, the current state of compilers (even a decade ago) is reasonably good at finding uninitialized uses across Translation Units. I've found myself several of these thanks to GCC.

Or in short: While this line seems unnecessary there's a good reason to keep it, which is called defensive programming, which is the foundation for the even more powerful tool called defense-in-depth …

db is a caller supplied variable. I would argue setting the member to NULL and thus avoiding use-after-free and double-free issues is worth it. Also note that static analyzers might catch such issues easily if all code is inside a single translation unit, but this is library code.

See how GCC can catch this across translation units:

$ cat extern.c #include <stdbool.h> void f(bool b, int *x) { /* I forgot to initialize x if !b. */ if (b) *x = 1; return; }

$ cat main.c #include <stdbool.h> #include <stdlib.h> void f(bool b, int *x); int main(void) { int x; f(0, &x); return x; }

$ cc -Wall -Wextra main.c extern.c -O3 -fanalyzer -flto main.c: In function ‘main’: main.c:12:16: warning: use of uninitialized value ‘x’ [CWE-457] [-Wanalyzer-use-of-uninitialized-value] 12 | return x; | ^ ‘main’: events 1-3 | | 6 | int main(void) | | ^ | | | | | (1) entry to ‘main’ | 7 | { | 8 | int x; | | ~ | | | | | (2) region created on stack here | 9 | | 10 | f(0, &x); | | ~ | | | | | (3) calling ‘f’ from ‘main’ | +--> ‘f’: events 4-6 | |extern.c:3:6: | 3 | void f(bool b, int *x) | | ^ | | | | | (4) entry to ‘f’ |...... | 6 | if (b) | | ~ | | | | | (5) following ‘false’ branch (when ‘b_2(D) == 0’)... | 7 | *x = 1; | 8 | return; | | ~ | | | | | (6) ...to here | <------+ | ‘main’: events 7-8 | |main.c:10:9: | 10 | f(0, &x); | | ^ | | | | | (7) returning to ‘main’ from ‘f’ | 11 | | 12 | return x; | | ~ | | | | | (8) use of uninitialized value ‘x’ here | main.c: In function ‘main’: main.c:8:13: warning: ‘x’ is used uninitialized [-Wuninitialized] 8 | int x; | ^

$ cc --version cc (Debian 12.2.0-14) 12.2.0 Copyright (C) 2022 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@hallyn @ikerexxe to NULL or not to NULL?

In general I would say NULL it, as the normal flow of the function does after fclose();.

It's a well understood idiom in c code, and it's a neat trick gcc is doing to warn us when we're doing it wrong, but that doesn't mean we should purposely do it wrongly. Someone else coming along and checking db->fp is not against the rules. Keeping db->fp non-null but closed, is.

@hallyn

The problem I see with setting to NULL is that it disables the ability of GCC to warn about such mistakes. You remove chances of writing Undefined Behavior, but I believe UB is good in some cases such as this one:

If the compiler can see UB, then it can know that you made a mistake, but if you transform it to defined behavior, it can't know anymore that you took a wrong decision, and it can only trust your decission. If we're sure that we can handle that NULL in a safe way (NULL is usually safe, because normally the worst thing that can happen is a crash, but we never know for sure), then it's fine to write a NULL. But if we want the compiler to detect our mistakes, we need to let it non-null but closed. To clarify, GCC should also catch a NULL dereference, since that's also UB, but if we don't NULL, it can catch even a simple copy.

As a disclaimer to my suggestion: GCC's fanalyzer isn't yet very mature, and may miss some cases of UB, so maybe we're safer with NULL for now. However, fanalyzer will only improve with time, and soon it will probably be much safer to not NULL, but just rely on the analyzer to make sure we didn't screw anything.

Since fanalyzer is far from perfect as of now, I'm neutral to the decision; just wanted to point out the benefits of letting it be non-null.

IMO there should be only two states for a pointer: valid && non-NULL or invalid && NULL. Having a invalid, but non-NULL pointer flying around is just asking for trouble.

And deref'ing a NULL ptr is UB on its own, thus -fsanitize=udefined should notice it, but -fsanitize=address most likely will (runtime-wise). And any sane linter/static code checker will flag missing NULL checks once it detects at least one NULL write to a location.

And if you are worried about GCC: There are attributes on a function argument level to tell GCC that a certain argument should not be NULL. That attribute is mostly for optimization, but will generally allow you to drop many NULL checks in favor of the compiler doing them at compile-time …

src/useradd.c

src/faillog.c

src/usermod.c

alejandro-colomar · 2023-01-31T00:19:01Z

src/usermod.c

 		goto out;

 	errno = 0;
 	last = strtoll(pos + 1, &pos, 10);
 	if (('\0' != *pos ) || (ERANGE == errno) ||
-	    (last != (unsigned long int)last))
+	    (last != (long long)(unsigned long int)last))
 		goto out;

 	if (first > last)


I can't link to the last lines of this function, but the casts there are useless and should probably be removed:

result.first = (unsigned long int)first; result.last = (unsigned long int)last;

The destination is of that type, so the conversion is automatic.

The casts check for truncation on architectures where long long and unsigned long are of different size.

I meant the casts like result.last = (unsigned long int)last; (not the ones written by you). Casts in an assignment will not add warnings.

src/login.c

src/groups.c

src/usermod.c

src/groups.c

src/faillog.c

alejandro-colomar · 2023-01-31T13:07:39Z

login: use strlcpy to always NULL terminate:

NULL refers to the null pointer constant. The null byte (a.k.a null character) is NUL (see ascii(7)).

https://man.archlinux.org/man/ascii.7

BenBE · 2023-01-31T19:14:53Z

lib/commonio.c

+		r = snprintf (buf, sizeof buf, "%s-", db->filename);
+		if (r < 0 || (size_t)r >= sizeof buf) {
+			(void) fclose (db->fp);
+			db->fp = NULL;


On the contrary: Given most memory is allocated uninitialized with malloc throughout the code base (bug in itself, but well), setting things to NULL actively prevents use-after-free issues (cleanly detectable runtime crash if not cought in the code). Which brings us to the second point: modern compilers allow to mark functions as requiring pointers to be non-NULL, thus forcing you to either check that assumption or pass it on in your library signature. This is very powerful, in itself and can find loads of memory issues even at compile-time. That aside, uninitialized memory is subject to ASAN, which together with LSAN can uncover many memory issues before they become critical bugs.

Or in short: While this line seems unnecessary there's a good reason to keep it, which is called defensive programming, which is the foundation for the even more powerful tool called defense-in-depth …

libmisc/failure.c

BenBE · 2023-01-31T19:25:19Z

src/chgpasswd.c

@@ -183,7 +183,13 @@ static void process_flags (int argc, char **argv)
 #if defined(USE_SHA_CRYPT) || defined(USE_BCRYPT) || defined(USE_YESCRYPT)
 		case 's':
 			sflg = true;
-                        bad_s = 0;
+			bad_s = 0;
+			if (!crypt_method) {


Unless all getopt implementations always reorder flags to process c before s this will fail for e.g. -s 666 -c YESCRYPT

BenBE · 2023-01-31T19:26:46Z

src/chgpasswd.c

+					 _("%s: --sha-rounds requires --crypt-method to be specified\n"),
+					 Prog);
+				usage (E_USAGE);
+			}
 #if defined(USE_SHA_CRYPT)
 			if (  (   ((0 == strcmp (crypt_method, "SHA256")) || (0 == strcmp (crypt_method, "SHA512")))


Maybe use stricmp for user convenience …

I guess stricmp() is another name for strcasecmp(3) is some system.

We could use strcasecmp(3), which is in POSIX.1. Here's an extract of the man page (in my git HEAD):

SYNOPSIS #include <strings.h> int strcasecmp(const char *s1, const char *s2); int strncasecmp(const char s1[.n], const char s2[.n], size_t n); DESCRIPTION The strcasecmp() function performs a byte‐by‐byte comparison of the strings s1 and s2, ignoring the case of the characters. [...] [...] STANDARDS POSIX.1‐2008. HISTORY 4.4BSD, POSIX.1‐2001. The strcasecmp() and strncasecmp() functions first appeared in 4.4BSD, [...]

src/faillog.c

src/usermod.c

alejandro-colomar · 2023-03-01T18:49:55Z

login: use strlcpy to always NULL terminate:

NULL refers to the null pointer constant. The null byte (a.k.a null character) is NUL (see ascii(7)).

https://man.archlinux.org/man/ascii.7

Ping.

src/usermod.c

alejandro-colomar · 2023-03-01T19:08:02Z

src/usermod.c

+	if (close (fd) != 0 && errno != EINTR) {
+		fprintf (stderr,
+		         _("%s: failed to copy the lastlog entry of user %lu to user %lu: %s\n"),
+		         Prog, (unsigned long) user_id, (unsigned long) user_newid, strerror (errno));


[u]intmax_t and %jd or %ju is shorter. Now that we require C11, we could use it.

src/usermod.c

hallyn · 2023-03-31T01:12:48Z

@cgzones can you please 'resolve' conversations as you address them, so we can know when to take a(nother) look?

libmisc/copydir.c

alejandro-colomar · 2023-04-05T18:39:23Z

I'm going to pick your patch about dropping alloca(3) for a patch set of mine simplifying alloc.h APIs.

usermod.c:2157:24: warning: dereference of possibly-NULL ‘user_groups’ [CWE-690] [-Wanalyzer-possible-null-dereference]

A crypt method needs to be specified before the rounds can set: #0 __strcmp_sse42 () at ../sysdeps/x86_64/multiarch/strcmp-sse4_2.S:227 shadow-maint#1 0x0000555555557755 in process_flags (argv=0x7fffffffe4d8, argc=3) at chgpasswd.c:188 shadow-maint#2 main (argc=3, argv=0x7fffffffe4d8) at chgpasswd.c:427 chgpasswd.c:188:42: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]

subordinateio.c:360:20: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 360 | range1 = (*(struct commonio_entry **) p1)->eptr; | ^ subordinateio.c:364:20: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 364 | range2 = (*(struct commonio_entry **) p2)->eptr; | ^ groupio.c:215:15: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 215 | if ((*(struct commonio_entry **) p1)->eptr == NULL) { | ^ groupio.c:218:15: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 218 | if ((*(struct commonio_entry **) p2)->eptr == NULL) { | ^ groupio.c:222:34: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 222 | u1 = ((struct group *) (*(struct commonio_entry **) p1)->eptr)->gr_gid; | ^ groupio.c:223:34: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 223 | u2 = ((struct group *) (*(struct commonio_entry **) p2)->eptr)->gr_gid; | ^ pwio.c:187:15: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 187 | if ((*(struct commonio_entry **) p1)->eptr == NULL) | ^ pwio.c:189:15: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 189 | if ((*(struct commonio_entry **) p2)->eptr == NULL) | ^ pwio.c:192:35: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 192 | u1 = ((struct passwd *) (*(struct commonio_entry **) p1)->eptr)->pw_uid; | ^ pwio.c:193:35: warning: cast discards 'const' qualifier from pointer target type [-Wcast-qual] 193 | u2 = ((struct passwd *) (*(struct commonio_entry **) p2)->eptr)->pw_uid; | ^

Allocate enough memory for the strings, two slashes and the NUL terminator.

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).

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).

write(2) may not write the complete given buffer. Add a wrapper to avoid short writes.

Check for arithmetic overflows when computing offsets to avoid file corruptions for huge UIDs.

Update wording and include errno description.

Avoid checking if the file exists before opening it. Resolves a CodeQL report of Time-of-check time-of-use filesystem race condition.

BenBE · 2023-04-11T16:13:32Z

lib/commonio.c

+		r = snprintf (buf, sizeof buf, "%s-", db->filename);
+		if (r < 0 || (size_t)r >= sizeof buf) {
+			(void) fclose (db->fp);
+			db->fp = NULL;


IMO there should be only two states for a pointer: valid && non-NULL or invalid && NULL. Having a invalid, but non-NULL pointer flying around is just asking for trouble.

And deref'ing a NULL ptr is UB on its own, thus -fsanitize=udefined should notice it, but -fsanitize=address most likely will (runtime-wise). And any sane linter/static code checker will flag missing NULL checks once it detects at least one NULL write to a location.

And if you are worried about GCC: There are attributes on a function argument level to tell GCC that a certain argument should not be NULL. That attribute is mostly for optimization, but will generally allow you to drop many NULL checks in favor of the compiler doing them at compile-time …

BenBE · 2023-04-11T16:19:04Z

src/faillog.c

@@ -97,8 +97,11 @@ static void print_one (/*@null@*/const struct passwd *pw, bool force)
 		return;
 	}

-	offset = (off_t) pw->pw_uid * sizeof (fl);
-	if (offset + sizeof (fl) <= statbuf.st_size) {
+	if (__builtin_mul_overflow(pw->pw_uid, sizeof (fl), &offset)) {


Use helper macro/inline function to concentrate compiler specifics/build-ins to central places.

vapier · 2023-04-11T22:41:10Z

there's a lot of commits in here, and it seems like debate is not uniform among them. trying to get everything merged in one go will take a lot more effort than splitting up into smaller PRs so the less controversial ones can be merged independently.

alejandro-colomar · 2023-11-17T16:45:03Z

Is all of this already merged via other PRs?

cgzones · 2023-12-11T17:21:44Z

Superseded by #871.

alejandro-colomar reviewed Jan 30, 2023

View reviewed changes

src/chage.c Outdated Show resolved Hide resolved

alejandro-colomar reviewed Jan 30, 2023

View reviewed changes

libmisc/failure.c Outdated Show resolved Hide resolved

alejandro-colomar reviewed Jan 30, 2023

View reviewed changes