The changes in 4273c43902 did not guard
macros in `gatomic.h` which use `glib_typeof`. This meant that when
552b8fd862 was committed, moving the
include of `<type_traits>` under such a guard, these macros were still
trying to use it. This broke the build of at least vte.
Fix this by guarding the API break in `gatomic.h` too.
The doc used different phrasing for the same thing, e.g. "if any thread"
vs "any other thread."
Also make it clear that trying to take a write lock while already having
a lock, or trying to take a read lock while having a write lock, is
undefined.
When included inside an `extern "C"` block, this causes build failures
that look something like:
/usr/include/c++/10/type_traits:2930:3: error: template with C linkage
2930 | template<typename _Fn, typename... _Args>
| ^~~~~~~~
../../disas/arm-a64.cc:20:1: note: ‘extern "C"’ linkage started here
20 | extern "C" {
| ^~~~~~~~~~
Commit 4273c43902 made this opt in for
projects which are defining `GLIB_VERSION_MIN_REQUIRED`, but the include
of `<type_traits>` via `gmacros.h` was not included in this. If we move
the include out to the places where `glib_typeof` is called, we can make
it covered by this macro too, and save a few consumers from FTBFSing.
That also means that, if you don't want to fix your use of the headers,
and as long as this version is sufficient for you, a quick workaround is
to define `GLIB_VERSION_MIN_REQUIRED` to `GLIB_VERSION_2_66` or lower.
Suggested by Simon McVittie.
Alternative to: https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1935
Fixes: https://gitlab.gnome.org/GNOME/glib/-/issues/2331
The `source_fds`/`target_fds` functionality is not supported on Windows
at the moment.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2097
This should introduce no functional changes, but condenses the variants
of the internal spawn implementation down to be more like `gspawn.c`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
I realise Windows uses handles rather than PIDs, but given that there
are multiple platform-specific implementations of the public
`g_spawn_*()` API, I think it is less confusing for them all to use the
same naming scheme.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This is a simple wrapper around the new source/target FD mapping
functionality in `fork_exec()`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
If `stdout_fd` was set to (say) 6, and `stderr_fd` was set to 1, the
`set_cloexec()` call for setting up `stderr` would set the new `stdout`
for the forked process to be closed in the pending `exec()`.
This would cause the child process to error when writing to `stdout`.
This situation happens when using `G_SUBPROCESS_FLAGS_STDERR_MERGE`.
Add some conditions to prevent setting `CLOEXEC` in such cases.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
It was previously possible to specify the FD number which
`child_err_report_fd` was assigned, as a target FD in the FD mapping set
up using `g_subprocess_launcher_take_fd()`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2097
This effectively moves some of the functionality of `GSubprocess`
(`g_subprocess_launcher_take_fd()`) into `g_spawn*()`, which should make
implementation a little simpler.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
This is an internal change which won’t affect the public API. It should
introduce no functional changes, but simplifies the code a little.
The arguments from `fork_exec_with_pipes()` have been added to
`fork_exec_with_fds()`. `child_close_fds` has been dropped since it’s
now an implementation detail within the function.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2097
Since !1715, g_atomic_pointer_get (&x) has usually returned the type of
x, rather than a generic pointer, in C++ code (where x is any pointer,
or any pointer-sized integer such as guintptr). glib/tests/cxx.cpp
asserts that this is the case.
However, this was only implemented for the lock-free fast-path, not
for the slow path used in platforms with an ARMv5 baseline (and
therefore no atomic instructions) such as Debian armel.
Signed-off-by: Simon McVittie <smcv@collabora.com>
GByteArray uses guint for storing the length of the byte array, but it
also has a constructor (g_byte_array_new_take) that takes length as a
gsize. gsize may be larger than guint (64 bits for gsize vs 32 bits
for guint). It is possible to call the function with a value greater
than G_MAXUINT, which will result in silent length truncation. This
may happen as a result of unreffing GBytes into GByteArray, so rather
be loud about it.
(Test case tweaked by Philip Withnall.)
This adds g_string_replace(), a function that replaces instances of one string
with another in a GString. It allows the caller to specify the maximum number
of replacements to perform, and returns the number of replacements performed
to the caller.
Fixes: #225
Unfortunately, `g_memdup()` accepts its size argument as a `guint`,
unlike most other functions which deal with memory sizes — they all use
`gsize`. `gsize` is 64 bits on 64-bit machines, while `guint` is only 32
bits. This can lead to a silent (with default compiler warnings)
truncation of the value provided by the caller. For large values, this
will result in the returned heap allocation being significantly smaller
than the caller expects, which will then lead to buffer overflow
reads/writes.
Any code using `g_memdup()` should immediately port to `g_memdup2()` and
check the pointer arithmetic around their call site to ensure there
aren’t other overflows.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2319
The public API `GIOChannel.line_term_len` is only a `guint`. Ensure that
nul-terminated strings passed to `g_io_channel_set_line_term()` can’t
exceed that length. Use `g_memdup2()` to avoid a warning (`g_memdup()`
is due to be deprecated), but not to avoid a bug, since it’s also
limited to `G_MAXUINT`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2319
Convert all the call sites which use `g_memdup()`’s length argument
trivially (for example, by passing a `sizeof()` or an existing `gsize`
variable), so that they use `g_memdup2()` instead.
In almost all of these cases the use of `g_memdup()` would not have
caused problems, but it will soon be deprecated, so best port away from
it
In particular, this fixes an overflow within `g_bytes_new()`, identified
as GHSL-2021-045 by GHSL team member Kevin Backhouse.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: GHSL-2021-045
Helps: #2319
This will replace the existing `g_memdup()` function, which has an
unavoidable security flaw of taking its `byte_size` argument as a
`guint` rather than as a `gsize`. Most callers will expect it to be a
`gsize`, and may pass in large values which could silently be truncated,
resulting in an undersize allocation compared to what the caller
expects.
This could lead to a classic buffer overflow vulnerability for many
callers of `g_memdup()`.
`g_memdup2()`, in comparison, takes its `byte_size` as a `gsize`.
Spotted by Kevin Backhouse of GHSL.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: GHSL-2021-045
Helps: #2319
Various tests have leaks where it isn't clear whether the data is
intentionally not freed, or leaked due to a bug. If we mark these
tests as TODO, we can skip them under AddressSanitizer and get the
rest to pass, giving us a baseline from which to avoid regressions.
Signed-off-by: Simon McVittie <smcv@collabora.com>
AddressSanitizer detects memory leaks, NULL parameters where only a
non-NULL parameter is expected, and other suspicious behaviour, so if
we try to test that sort of thing we can expect it to fail.
Signed-off-by: Simon McVittie <smcv@collabora.com>
We preallocate buffers that are used after forked. That is because
malloc()/free() are not async-signal-safe and must not be used between
fork() and exec().
However, for the child process that exits without fork, valgrind wrongly
reports these buffers as leaked.
That can be suppressed with "--child-silent-after-fork=yes", but it is
cumbersome.
Work around by trying to allocate the buffers on the stack. At
least in the common cases where the pointers are small enough
so that we can reasonably do that.
If the buffers happen to be large, we still allocate them on the heap
and the problem still happens. Maybe we could have also allocated them
as thread_local, but currently glib doesn't use that.
[smcv: Cosmetic adjustments to address review comments from pwithnall]
For manual test coverage that would reproduce the bug fixed in !1902,
copy /bin/true (or any other harmless executable) to
/usr/bin/spawn-test-helper.
Signed-off-by: Simon McVittie <smcv@collabora.com>
do_exec() and g_execute() rely on being passed a NULL search path
if we intend to avoid searching the PATH, but since the refactoring
in commit 62ce66d4, this was never done. This resulted in some spawn
calls searching the PATH when it was not intended.
Spawn calls that go through the posix_spawn fast-path were unaffected.
The deprecated gtester utility, as used in GTK 3, relies on the
ability to run an executable from the current working directory by
omitting the G_SPAWN_SEARCH_PATH flag. This *mostly* worked, because
our fallback PATH ends with ".". However, if an executable of the
same name existed in /usr/bin or /bin, it would run that instead of the
intended test: in particular, GTK 3's build-time tests failed if
ImageMagick happens to be installed, because gtester would accidentally
run display(1) instead of testsuite/gdk/display.
Fixes: 62ce66d4 "gspawn: Don’t use getenv() in async-signal-safe context"
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=977961
That changes the return type of functions like g_object_ref() that can
break C++ applications like Webkit. Note that it is not an ABI break.
It must thus be opt-in the same way we did when adding this to
g_object_ref() for GNU C compilers in the first place. Unfortunately it
cannot be done directly in gmacros.h because GLIB_VERSION_2_68 is not
defined there, and gversionmacros.h cannot be included there because
there is some strict ordering in which those headers must be included.
This means that applications that does not define
GLIB_VERSION_MIN_REQUIRED will still get an API break, so we encourage
them to declare their minimum requirement to avoir such issues in the
future too.
clang++ checks the 2nd args of __atomic_compare_exchange_n() has the
same type as the first, which fails when 2nd arg is nullptr which is of
type nullptr_t.
Ideally it should do `glib_typeof (*(atomic)) gapcae_oldval = (oldval);`
to ensure oldval and atomic have compatible types but unfortunately that
does not work neither.
Since that function never has been typesafe, and it is not even
attempting to use glib_typeof in case __ATOMIC_SEQ_CST is not defined,
drop it in __atomic_ case too.
Fixes issue #2226.
There are various places glib uses __typeof__ for type safety, but
that's a GNUC extension. C++11 has standard decltype() that does a
similar job, at least for cases we care about.
This avoids C++ code to always have to cast return value of
g_object_ref() which was causing type kind of error:
error: invalid conversion from ‘gpointer’ {aka ‘void*’} to
‘GstElementFactory*’ {aka ‘_GstElementFactory*’} [-fpermissive]
I found myself wanting to know the test that is currently being run,
where e.g. __func__ would be inconvenient to use, because e.g. the place
the string was needed was not in the test case function. Using __func__
also relies on the test function itself containing the whole path, while
loosing the "/" information that is part of the test path.
Since Windows 10 1607, we can make use of SetThreadDescription() API
for setting thread name. Unlike previously used exception based
method, this API will preserve configured thread name on dump file.
This makes it a little easier to link to in the generated documentation,
and separates it from the section above.
Link to the heading from the documentation for
`G_DEFINE_EXTENDED_ERROR`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
On FreeBSD it always crashes due to the platform’s `vasprintf()`
implementation being less forgiving than Linux’s. That’s fine.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This is mostly to cover historic code, but also includes a couple of
additional tests for extended error domains (see #14).
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
At this point, if `dest` is non-`NULL` then we can guarantee
`*dest != NULL` due to `g_propagate_error()` succeeding.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
When loading a GKeyFile, the sequence is usually:
keyfile = g_key_file_new();
g_key_file_load_xxx(keyfile, ...)
g_key_file_new() calls g_key_file_init(), which allocates a parse_buffer
for parsing. g_key_file_load_xxx() will then g_key_file_clear() the
keyfile and call g_key_file_init() again.
Just don't allocate a parse_buffer unless we need it for parsing.
If no pointer to a GError* has been passed to public API, there's not
need to look at translations via gettext or format an error message that
g_set_error_literal will entirely ignore in the end.
Factor out the GError creation to a common function. When adding a
support for extended error types, this will limit the number of places
where these errors are allocated.
Realistically any date over 200 bytes long is not going to be valid, so
limit the input length so we can’t spend too long doing UTF-8 validation
or normalisation.
oss-fuzz#28718
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
On heavily loaded CI machines it seems to be taking about 1.3s between
one `g_get_monotonic_time()` call and the next. Allow that.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Occasionally this test fails in CI with the message:
```
assertion failed: (current_time / 1000000 - last_time / 1000000 == 1)
```
The way this calculation is done at the moment, a difference of 1001ms
between `current_time` and `last_time` can result in failure, if the
times are close to a multiple to 1000ms.
Change it to only truncate the result after doing the subtraction, and
add a 500ms tolerance to account for scheduling delays in the test. (For
example, the `test_func()` could be called, then descheduled before it
gets to call `g_get_monotonic_time()`.
Additionally, change the test to use `g_assert_cmpint()` so that future
failures provide more useful debug information.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Instead, return `NULL` if the checksum type is unsupported. This may
come in useful if we have to withdraw support for a particular checksum
type in future, due to it being broken.
These semantics were already in place for the return value of
`g_checksum_new()` — see commit 877cc60f and bug
comment https://bugzilla.gnome.org/show_bug.cgi?id=501853#c6.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
This commit only looks at the `Returns:` lines in the documentation, and
has examined all of them in the file. Function arguments have not been
checked.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2227
This commit only looks at the `Returns:` lines in the documentation, and
has examined all of them in the file. Function arguments have not been
checked.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2227
