lib/cast.h: const_cast(): Add macro for dropping 'const'
Uses of this macro indicate a code smell, but in some cases, libc
functions require breaking const correctness. Use this macro to wrap
casts in such cases, so that we limit the danger of the cast.
It only permits discarding const. Discarding any other qualifiers, or
doing other type changes should result in a compile-time error.
Fixes: ef95bb7ed139 ("src/su.c: Fix type of variable") Closes: <https://github.com/shadow-maint/shadow/issues/915> Reported-by: Sam James <sam@gentoo.org> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
strtou[l]l(3) silently converts negative numbers into positive. This
behavior is wrong: a negative value should be parsed as a negative
value, which would underflow unsigned (long) long, and so would return
the smallest possible value, 0, and set errno to ERANGE to report an
error.
src/sulogin.c: Remove 'static' from local variable, but keep initialization
We don't need 'static', because it's in main(), which is only called
once. However, we will need initialization as if it were 'static', so
use ={} to initialize it. This will allow freeing the pointers before
they have been allocated.
ISO C guarantees that #undef is a no-op if there is no such macro.
C11::6.10.3.5p2:
> A preprocessing directive of the form
>
> # undef identifier new-line
>
> causes the specified identifier no longer to be defined as a macro
> name. It is ignored if the specified identifier is not currently
> defined as a macro name.
lib/: Remove incorrect /*@out@*/ comment from functions that read the pointee
These functions (e.g., gr_free()), explicitly dereference the pointer
and read the pointee.
The /@out@/ comment, which is (almost) analogous to the
[[gnu::access(write_only, ...)]] attribute, means that the pointee can
be uninitialized, since it won't read it. There's a difference between
/@out@/ and the GCC attribute: the attribute doesn't require that the
call writes to the pointee, while /@out@/ requires that the pointee be
fully initialized after the call, so it _must_ write to it.
A guess of why it was used is that these functions are similar to
free(3), which does not read the memory it frees, and so one would
assume that if it doesn't read, write_only (or equivalents) are good.
That's wrong in several ways:
- free(3) does not read _nor_ write to the memory, so it would
be slightly inappropriate to use write_only with it. It wouldn't be
"wrong", but [[gnu::access(none, ...)]] would be more appropriate.
- Because /@out@/ requires that the call writes to the pointee, it
would be wrong to use it in free(3), which doesn't write to the
pointee.
- Our functions are similar to free(3) conceptually, but they don't
behave like free(3), since they do read the memory (pointee) (and
also write to it), and thus they're actually read_write.
Samanta Navarro [Fri, 12 Jan 2024 11:49:27 +0000 (11:49 +0000)]
lib/sgetgrent.c: fix null pointer dereference
If reallocation fails in function list, then reset the size to 0 again.
Without the reset, the next call assumes that `members` points to
a memory location with reserved space.
Also use size_t instead of int for size to prevent signed integer
overflows. The length of group lines is not limited.
The size of time_t varies across systems, but since data type long is
more than enough to calculate with days (precision of shadow file),
use it instead.
Just in case a shadow file contains huge values, check for a possible
signed integer overflow.
lib/getdef.c: Reject negative values in getdef_* except -1
The values are retrieved from login.defs files, which normally do not
contain negative values. In fact, negative value -1 is used in many
code places as "feature disabled", which is normally achieved by
simply commenting out the key from the file.
The variable declarations for the buffers have been aligned in this
commit, so that they appear in the diff, making it easier to review.
Some important but somewhat tangent changes included in this commit:
- lib/nss.c: The size was being defined as 65, but then used as 64.
That was a bug, although not an important one; we were just wasting
one byte. Fix that while we replace snprintf() by SNPRINTF(), which
will get the size from sizeof(), and thus will use the real size.
These functions are like [v]snprintf(3), but return -1 on truncation,
which makes it easier to test. In fact, the API of swprintf(3), which
was invented later than snprintf(3), and is the wide-character version
of it, is identical to this snprintf_().
snprintf(3) is iseful in two cases:
- We don't care if the output is truncated. snprintf(3) is fine for
those, and the return value can be ignored. But snprintf_() is also
fine for those.
- Truncation is bad. In that case, it's as bad as a hard error (-1)
from snprintf, so merging both problems into the same error code
makes it easier to handle errors. Return the length if no truncation
so that we can use it if necessary.
Not returning the whole length before truncation makes a better API,
which need not read the entire input, so it's less vulnerable to DoS
attacks when a malicious user controls the input.
It wraps snprintf(3) so that it performs some steps that one might
forget, or might be prone to accidents:
- It calculates the size of the destination buffer, and makes sure it's
an array (otherwise, using sizeof(s) would be very bad).
- It calculates if there's truncation or an error, returning -1 if so.
BTW, this macro doesn't have any issues of double evaluation, because
sizeof() doesn't evaluate its argument (unless it's a VLA, but then the
static_assert(3) within NITEMS() makes sure VLAs are not allowed).
This macro is very similar to STRTCPY(), defined in
<lib/string/strtcpy.h>.
commonio.c: In function 'commonio_unlock':
commonio.c:487:49: warning: '.lock' directive output may be truncated writing 5 bytes into a region of size between 1 and 1024 [-Wformat-truncation=]
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~
commonio.c:487:17: note: 'snprintf' output between 6 and 1029 bytes into a destination of size 1024
487 | snprintf (lock, sizeof lock, "%s.lock", db->filename);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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.
su.c:678:26: warning: format ‘%s’ expects argument of type ‘char *’, but argument 4 has type ‘const void *’ [-Wformat=]
su.c:681:44: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
su.c:683:46: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘const void *’ [-Wformat=]
Reported-by: Christian Göttsche <cgzones@googlemail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
lib/, src/: snprintf(3) already terminates strings with NUL
We don't need to terminate them manually after the call. Remove all
that paranoid code, which in some cases was even wrong. While at it,
let's do a few more things:
- Use sizeof(buf) for the size of the buffer. I found that a few cases
were passing one less byte (probably because the last one was
manually zeroed later). This caused a double NUL. snprintf(3) wants
the size of the entire buffer to properly terminate it. Passing the
exact value hardcoded is brittle, so use sizeof().
- Align and improve style of variable declarations. This makes them
appear in this diff, which will help review the patch.
lib/: Use ATTR_STRING() on stpecpy() and strtcpy()
These functions consume a source string. Document that. There's no way
to mark that they also produce a string in dst, though. That will be up
to the static analyzer to guess.
lib/, src/: Fix error handling after strto[u]l[l](3)
- Set errno = 0 before the call. Otherwise, it may contain anything.
- ERANGE is not the only possible errno value of these functions. They
can also set it to EINVAL.
- Any errno value after these calls is bad; just compare against 0.
- Don't check for the return value; just errno. This function is
guaranteed to not modify errno on success (POSIX).
- Check endptr == str, which may or may not set EINVAL.
lib/chkname.c: Use tmp variable to avoid a -Wsign-compare warning
I used size_t because:
sysconf(3) can return -1 if the value is not supported, but then it can
only mean that there's no limit. Having no limit is the same as having
a limit of SIZE_MAX (to which -1 is converted).
lib/env.c: Replace strncpy(3) call by stpcpy(mempcpy(), "")
We were using strncpy(3), which is designed to copy from a string into a
(null-padded) fixed-size character array. However, we were doing the
opposite: copying from a known-size array (which was a prefix of a
string), into a string. That's why we had to manually zero the buffer
afterwards.
Use instead mempcpy(3) to copy the non-null bytes, and then terminate
with a null byte with stpcpy(..., "").
Cc: "Serge E. Hallyn" <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
Upcoming `gcc-14` enabled a few warnings into errors, like
`-Wimplicit-function-declaration`. This caused `shadow` build to fail
as:
pwunconv.c: In function 'main':
pwunconv.c:132:13: error: implicit declaration of function 'getdef_bool' [-Wimplicit-function-declaration]
132 | if (getdef_bool("USE_TCB")) {
| ^~~~~~~~~~~
lib/: Use NITEMS() instead of SIZEOF_ARRAY() where number of elements is meant
For arrays of char, both NITEMS() and SIZEOF_ARRAY() return the same
value. However, NITEMS() is more appropriate. Think of wide-character
equivalents of the same code; with NITEMS(), they would continue to be
valid, while with SIZEOF_ARRAY(), they would be wrong.
In the implementation of ZUSTR2STP(), we want SIZEOF_ARRAY() within the
static assert, because we're just comparing the sizes of the source and
destination buffers, and we don't care if we compare sizes or numbers of
elements, and using sizes is just simpler. But we want NITEMS() in the
zustr2stp() call, where we want to copy a specific number of characters.
The maximum length of 32 wasn't being enforced in the code, and POSIX
doesn't specify that maximum length either, so it seems it was an
arbitrary limit of the past that doesn't exist any more. Use a regex
that has no length limit.
lib/strncpy.h: Add STRNCPY() wrapper for strncpy(3)
This wrapper calculates the destination buffer's size, to avoid errors
in the size calculation.
A curious fact: this macro did exist in Version 7 Unix (with a slightly
different name). I found it by chance, investigating the origins of
strncpy(3) and strncat(3) in V7, after Branden suggested me to do so,
related to recent discussions about string_copying(7).
alx@debian:~/src/unix/unix/Research-V7$ grepc SCPYN .
./usr/src/cmd/login.c:#define SCPYN(a, b) strncpy(a, b, sizeof(a))
Our implementation is slightly better, because using nitems() we're
protected against passing a pointer instead of an array, and it's also
conceptually more appropriate: for wide characters, it would be
lib/: Remove off-by-one bugs in calls to strncpy(3)
We're not even zeroing the last byte after this call. This was a
completely gratuitous truncation of one byte, and the resulting
character array still wasn't guaranteed to be null terminated, because
strncpy(3) can't do that.
Just to clarify, none of these structures needed zeroing, as they are
treated as null-padded fixed-size character arrays. Calling strncpy(3)
was actually the correct call, and the only problem was unnecessarily
truncating strings by one byte more than necessary.
Cc: Matthew House <mattlloydhouse@gmail.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
projects / thirdparty / shadow.git / log
