We mishandled the case where the size we read from the file actually
matched the maximum size fully. In that case we cannot really make a
determination whether the file was fully read or only partially. In that
case let's do another loop, so that we operate with a buffer, and
we can detect the EOF (which will be signalled to us via a short read).
There's a very gradual increase of anonymous memory in systemd-journald that
blames to 2ac67221bb.
systemd-journald makes many calls to read /proc/PID/cmdline and
/proc/PID/status, both of which tend to be well under 4K. However the
combination of allocating 4M read buffers, then using `realloc()` to
shrink the buffer in `read_virtual_file()` appears to be creating
fragmentation in the heap (when combined with the other allocations
systemd-journald is doing).
To help mitigate this, try reading /proc with a 4K buffer as
`read_virtual_file()` did before 2ac67221bb.
If it isn't big enough then try again with the larger buffers.
When reading virtual files (i.e. procfs, sysfs, …) we currently put a
limit of 4M-1 on that. We have to pick something, and we have to read
these files in a single read() (since the kernel generally doesn't
support continuation read()s for them). 4M-1 is actually the maximum
size the kernel allows for reads from files in /proc/sys/, all larger
reads will result in an ENOMEM error (which is really weird, but the
kernel does what the kernel does). Hence 4M-1 sounds like a smart
choice.
However, we made one mistake here: in order to be able to detect EOFs
properly we actually read one byte more than we actually intend to
return: if that extra byte can be read, then we know the file is
actually larger than our limit and we can generate an EFBIG error from
that. However, if it cannot be read then we know EOF was hit, and we are
good. So ultimately after all we issued a single 4M read, which the
kernel then responds with ENOMEM to. And that means read_virtual_file()
actually doesn't work properly right now on /proc/sys/. Let's fix that.
The fix is simple, lower the limit of the the buffer we intend to return
by one, i.e. 4M-2. That way, the read() we'll issue is exactly as large
as the limit the kernel allows, and we still get safely detect EOF from
it.
Apparently people use such large key files. Specifically, people used 4M
key files, and we lowered the limit from 4M to 4M-1 back in 248.
This raises the limit to 64M for read_full_file() to avoid these
specific issues and give some non-trivial room beyond the 4M files seen
IRL.
Note that that a 64M allocation in glibc is always immediately done via
mmap(), and is thus a lot slower than shorter allocations. This means
read_virtual_file() becomes ridiculously slow if we'd use the large
limit, since we use it all the time for reading /proc and /sys metadata,
and read_virtual_file() typically allocates the full size with malloc()
in advance. In fact it becomes so slow, that test-process-util kept
timing out on me all the time, once I blindly raised the limit.
This patch hence introduces two distinct limits for read_full_file() and
read_virtual_file(): the former is much larger than the latter and the
latter remains where it is. This is safe since the former uses an
exponentially growing realloc() loop while the latter uses the
aforementioend ahead-of-time full limit allocation.
Fixes: #19193
We recently started making more use of malloc_usable_size() and rely on
it (see the string_erase() story). Given that we don't really support
sytems where malloc_usable_size() cannot be trusted beyond statistics
anyway, let's go fully in and rework GREEDY_REALLOC() on top of it:
instead of passing around and maintaining the currenly allocated size
everywhere, let's just derive it automatically from
malloc_usable_size().
I am mostly after this for the simplicity this brings. It also brings
minor efficiency improvements I guess, but things become so much nicer
to look at if we can avoid these allocation size variables everywhere.
Note that the malloc_usable_size() man page says relying on it wasn't
"good programming practice", but I think it does this for reasons that
don't apply here: the greedy realloc logic specifically doesn't rely on
the returned extra size, beyond the fact that it is equal or larger than
what was requested.
(This commit was supposed to be a quick patch btw, but apparently we use
the greedy realloc stuff quite a bit across the codebase, so this ends
up touching *a*lot* of code.)
It's a wrapper around malloc_usable_size() that is supposed to be
compatible with _FORTIFY_SOURCES=1, by taking the
__builtin_object_size() data into account, the same way as the
_FORTIFY_SOURCES=1 logic does.
Fixes: #19203
HAVE_SMACK_RUN_LABEL was dropped back in 348b44372f,
so one line in etc.conf was not rendered as expected ;(
Checking if names are defined is paying for itself!
Since the new functionality is controlled by an option, this causes no change
in output yet, except tests.
The login in the old branch of !(flags & PROCESS_CMDLINE_QUOTE) is essentially
unmodified. But there is an important difference in behaviour: instead of
unconditionally reading the whole virtual file, we now read only 'max_columns'
bytes. This makes out code to write process lists quite a bit more efficient
when there are processes with long command lines.
This fixes maybe-uninitialized warning:
```
../src/basic/fileio.c: In function ‘chase_symlinks_and_fopen_unlocked’:
../src/basic/fileio.c:1026:19: warning: ‘f’ may be used uninitialized in this function [-Wmaybe-uninitialized]
1026 | *ret_file = f;
| ~~~~~~~~~~^~~
```
[11/657] Compiling C object src/basic/libbasic.a.p/fileio.c.o
../src/basic/fileio.c: In function ‘write_string_stream_ts’:
../src/basic/fileio.c:167:21: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized]
167 | if (futimens(fd, twice) < 0)
| ^~~~~~~~~~~~~~~~~~~
This code is trying to do two things: when reading a file with working
st.st_size, detect when the file size changes between the fstat() and our
allocation of the buffer based on the returned size, and the subsequent read().
When reading a file without st.st_size, read up to READ_FULL_BYTES_MAX.
But this second scenario was partially broken: we'd start with size = 4095, and
double the size up to three times, i.e. up to 32767. But we want to read up to
READ_FULL_BYTES_MAX.
So let's listentangle the two cases a bit: if a file returns non-zero st._size,
proceed as before. But if we don't know the size, let's immediately allocate
the buffer of maximum size of READ_FULL_BYTES_MAX. I think that allocating 4MB
and 1MB is going to take pretty much the same time as long as the memory is not
written to, so by allocating 1MB, 2MB, and 4MB, we wouldn't really be saving
anything internally, but wasting time on repeated reads, if the file is long
enough.
Also, don't do the seek if we know we're going to return an error immediately
after.
This should fix reading of any files in /proc, which all have size == 0. In
particular, various files read by coredump might be larger than 32767.
What about /sys? The file there return a fake value, usually 4096. So we'll
allocate a small buffer and read that.
We'd proceed rather inefficiently: the initial buffer size was LINE_MAX/2,
i.e. only 1k. We can read 4k at the same cost.
Also, we'd try to allocate 1025, 2049, 4097 bytes, i.e. always one higher than
the power-of-two size. Effectively the allocation would be bigger, and we'd
waste the additional space. So let's allocate aligned to the power-of-two size.
size=4095, 8191, 16383, so we allocate 4k, 8k, 16k.
We'd first assign a value up to SSIZE_MAX, and then immediately check if we
have a value bigger than READ_FULL_BYTES_MAX. This wasn't exactly wrong, but a
bit roundabout. Let's immediately assign the value from the appropriate range
or error out.
Coverity CID#1450973.
We aren't interested in the data previousl read, hence free() followed
by malloc() is typically better since it means libc doesn't have to
restore the contained data needlessly.
It's so similar to copy_access(), hence let's move it over and rename it
in similar style to the rest of the functions.
No change in behaviour, just moving things over.
If the cleanup function returns the appropriate type, use that to reset the
variable. For other functions (usually the foreign ones which return void), add
an explicit value to reset to.
This causes a bit of code churn, but I think it might be worth it. In a
following patch static destructors will be called from a fuzzer, and this
change allows them to be called multiple times. But I think such a change might
help with detecting unitialized code reuse too. We hit various bugs like this,
and things are more obvious when a pointer has been set to NULL.
I was worried whether this change increases text size, but it doesn't seem to:
-Dbuildtype=debug:
before "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494520 Feb 16 15:06 build/systemd*
after "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494576 Feb 16 15:10 build/systemd*
now:
-rwxrwxr-x 1 zbyszek zbyszek 4117672 Feb 16 14:36 build/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 4494640 Feb 16 15:15 build/systemd*
-Dbuildtype=release:
before "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:09 build-rawhide/systemd*
after "tree-wide: return NULL from freeing functions":
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:10 build-rawhide/systemd*
now:
-rwxrwxr-x 1 zbyszek zbyszek 5252256 Feb 14 14:47 build-rawhide/libsystemd.so.0.30.0*
-rwxrwxr-x 1 zbyszek zbyszek 1834184 Feb 16 15:16 build-rawhide/systemd*
I would expect that the compiler would be able to elide the setting of a
variable if the variable is never used again. And this seems to be the case:
in optimized builds there is no change in size whatsoever. And the change in
size in unoptimized build is negligible.
Something strange is happening with size of libsystemd: it's bigger in
optimized builds. Something to figure out, but unrelated to this patch.
This beefs up the READ_FULL_FILE_CONNECT_SOCKET logic of
read_full_file_full() a bit: when used a sender socket name may be
specified. If specified as NULL behaviour is as before: the client
socket name is picked by the kernel. But if specified as non-NULL the
client can pick a socket name to use when connecting. This is useful to
communicate a minimal amount of metainformation from client to server,
outside of the transport payload.
Specifically, these beefs up the service credential logic to pass an
abstract AF_UNIX socket name as client socket name when connecting via
READ_FULL_FILE_CONNECT_SOCKET, that includes the requesting unit name
and the eventual credential name. This allows servers implementing the
trivial credential socket logic to distinguish clients: via a simple
getpeername() it can be determined which unit is requesting a
credential, and which credential specifically.
Example: with this patch in place, in a unit file "waldo.service" a
configuration line like the following:
LoadCredential=foo:/run/quux/creds.sock
will result in a connection to the AF_UNIX socket /run/quux/creds.sock,
originating from an abstract namespace AF_UNIX socket:
@$RANDOM/unit/waldo.service/foo
(The $RANDOM is replaced by some randomized string. This is included in
the socket name order to avoid namespace squatting issues: the abstract
socket namespace is open to unprivileged users after all, and care needs
to be taken not to use guessable names)
The services listening on the /run/quux/creds.sock socket may thus
easily retrieve the name of the unit the credential is requested for
plus the credential name, via a simpler getpeername(), discarding the
random preifx and the /unit/ string.
This logic uses "/" as separator between the fields, since both unit
names and credential names appear in the file system, and thus are
designed to use "/" as outer separators. Given that it's a good safe
choice to use as separators here, too avoid any conflicts.
This is a minimal patch only: the new logic is used only for the unit
file credential logic. For other places where we use
READ_FULL_FILE_CONNECT_SOCKET it is probably a good idea to use this
scheme too, but this should be done carefully in later patches, since
the socket names become API that way, and we should determine the right
amount of info to pass over.
