If an array with more than INT_MAX elements is passed to functions
internally calling g_build_path_va or g_build_pathname_va, then a
signed integer overflow and eventual out of boundary read access can
occur.
Use size_t instead of gint for lengths and array sizes.
This adds cross-platform support for it: on glibc, musl and BSD’s libc,
the flag is natively supported. On Windows, convert it to the `N` flag,
which similarly indicates that an open file shouldn’t be inherited by
child processes.
This allows us to unconditionally pass `e` to `g_fopen()` so `O_CLOEXEC`
can easily be set on its FDs.
Also do the same for `g_freopen()`, since it shares the same underlying
mode handling code.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
If g_file_set_contents{_full,} is replacing an existing file, require
that the tmpfile have the same mode as the existing file.
This prevents the umask from taking effect for consistent writes to
existing files.
ClosesGNOME/dconf#76
Otherwise it’s easy to assume that they create a file in the system
temporary directory, if they’re only called with a filename template
(without a path).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This assertion basically mirrors the existing `buffer_size >= 2`
assertion (one implies the other), but scan-build doesn’t seem to be
able to work that out — so give it some help.
Fixes:
```
../../../glib/gfileutils.c: In function ‘g_get_current_dir’:
../../../glib/gfileutils.c:2995:17: warning: potential null pointer dereference [-Wnull-dereference]
2995 | buffer[1] = 0;
| ~~~~~~~~~~^~~
../../../glib/gfileutils.c:2994:17: warning: potential null pointer dereference [-Wnull-dereference]
2994 | buffer[0] = G_DIR_SEPARATOR;
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Basically various trivial instances of the following MSVC compiler
warning:
```
../gio/gio-tool-set.c(50): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Partially because the use of `G_GSIZE_MODIFIER` breaks translatable
string extraction (issue #3271) and partially because `g_format_size()`
produces more human-readable results anyway.
Prior to commit cf5e371c67 the code was using `%lu`, so this is a fairly
new issue.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3271
Move it to a separate page so more detail can be provided about all the
groups of API.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3037
On certain platforms where file size (off_t) can be truncated when assigning to
gsize, get_contents_regfile() may use the truncated size as the size to read. Add
an explicit check to raise an error in such cases.
G_FILE_ERROR_FAILED will be raised in this case, aligning with behavior for other
cases.
This generally affects 32-bit non-Win32 platforms.
When calling `g_file_set_contents_full()` without
`G_FILE_SET_CONTENTS_CONSISTENT`, the file is written by opening it,
`write()`ing to it, then closing it.
This is fine as long as the file is not longer than the new content you
want to set its contents to. If it is, the last bit of the old content
remains, because `g_file_set_contents_full()` was missing an
`ftruncate()` call.
Fix that, and change the tests to catch truncation failures in future.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #3144
On 32 bit systems 'long' value will overflow in 2038 and become negative.
As it is used to index into letters array, and % operation preserves signs,
data corruption will then occur.
Signed-off-by: Alexander Kanavin <alex@linutronix.de>
In practice, this will never happen.
If `getcwd()` returns `ERANGE` whenever the working directory is ≥
`PATH_MAX`, though, the previous implementation of the loop would run
until `max_len == G_MAXULONG`, and would then overflow when adding `1`
to it for a nul terminator in the allocation.
Avoid that problem by always keeping `buffer_len` as a power of two, and
relying on `getcwd()` to write a nul terminator within the buffer space
it’s given. It seems to do this on all platforms we care about, because
the previous version of the code never explicitly wrote a nul terminator
anywhere.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #98
mkstemp-like family of functions also use g_open () under the hood so
they should pass the O_CLOEXEC flag there for race-free setting of the
close-on-exec flag.
The sample code here wasn't a race-free version of the race-susceptible
anti-pattern, because it would have dereferenced a symlink automatically.
Fixes: 293b4923 "Clarify g_file_test() docs about TOCTOU bugs"
Signed-off-by: Simon McVittie <smcv@collabora.com>
Do not use 4-spaces indentation for return and argument descriptions, to
avoid getting rendered as preformatted blocks.
Annotate code examples with their language, for syntax highlighting.
Isolate the first paragraph to give a short description.
Do not show just what not to do: show what to do instead, otherwise
people won't know how to fix their code.
Make sure to link to an explanation of the TOCTOU class of bugs;
Wikipedia is as good a place as any.
It’s entirely possible that `g_file_read_link()` will return a relative
path. Mention that in the documentation, and include a short example of
how to make the path absolute for further computation.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Add SPDX license (but not copyright) headers to all files which follow a
certain pattern in their existing non-machine-readable header comment.
This commit was entirely generated using the command:
```
git ls-files glib/*.[ch] | xargs perl -0777 -pi -e 's/\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/\n \*\n \* SPDX-License-Identifier: LGPL-2.1-or-later\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/igs'
```
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1415
The output pointer must not go past the ending \0.
warning: HEAP[testglib.exe]:
warning: Heap block at 0000011EA35745A0 modified at 0000011EA35745BF past requested size of f
Fixes commit 9a30a495ec "gfileutils: Improve performance of g_canonicalize_filename()"
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
This code was skipping fsync on BTRFS because of an old guarantee about
the overwrite-by-rename behavior that no longer holds true. This has
been confirmed by the BTRFS developers to no longer be guaranteed since
Kernel 3.17 (August 2014), but it was guaranteed when this optimization
was first introduced in 2010.
This could result in empty files after crashes in applications using
g_file_set_contents(). Most prominently this might have been the cause
of dconf settings getting lost on BTRFS after crashes due to the
frequency with which such writes can happen in dconf.
See: https://gitlab.gnome.org/GNOME/dconf/-/issues/73
If a path starts with more than two slashes, the `start` value was
previously incorrect:
1. As per the `g_path_skip_root()` call, `start` was set to point to
after the final initial slash. For a path with three initial
slashes, this is the character after the third slash.
2. The canonicalisation loop to find the first dir separator sets
`output` to point to the character after the first slash (and it
overwrites the first slash to be `G_DIR_SEPARATOR`).
3. At this point, with a string `///usr`, `output` points to the second
`/`; and `start` points to the `u`. This is incorrect, as `start`
should point to the starting character for output, as per the
original call to `g_path_skip_root()`.
4. For paths which subsequently include a `..`, this results in the
`output > start` check in the `..` loop below not skipping all the
characters of a preceding path component, which is then caught by
the `G_IS_DIR_SEPARATOR (output[-1])` assertion.
Fix this by resetting `start` to `output` after finding the final slash
to keep in the output, but before starting the main parsing loop.
Relatedly, split `start` into two variables: `after_root` and
`output_start`, since the variable actually has two roles in the two
parts of the function.
Includes a test.
This commit is heavily based on suggestions by Sebastian Wilhemi and
Sebastian Dröge.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
oss-fuzz#41563
Improve the performance of canonicalising filenames with many `..` or
`.` components, by modifying the path inline rather than calling
`memmove()`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2541
glib/gfileutils.c: In function 'g_file_test':
glib/gfileutils.c:375:18: warning: comparison of integer expressions of different signedness: 'int' and 'long unsigned int'
if (attributes == INVALID_FILE_ATTRIBUTES)
^~
glib/gfileutils.c: In function 'g_get_current_dir':
glib/gfileutils.c:2882:40: warning: comparison of integer expressions of different signedness: 'DWORD' {aka 'long unsigned int'} and 'int'
if (GetCurrentDirectoryW (len, wdir) == len - 1)
^~
This is basically glnx_steal_fd() from libglnx. We already had two
private implementations of it in GLib.
Signed-off-by: Simon McVittie <smcv@collabora.com>
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.
Behaviour in that case is implementation-defined and how many bytes were
actually written can't be expressed by the return value anymore.
Instead do a short write of G_MAXSSIZE bytes and let the existing loop
for handling short writes takes care of the remaining length.
glib/gfileutils.c: In function ‘write_to_file’:
glib/gfileutils.c:1176:19: error: comparison of integer expressions of different signedness: ‘gssize’ {aka ‘long int’} and ‘gsize’ {aka ‘long unsigned int’} [-Werror=sign-compare]
1176 | g_assert (s <= length);
| ^~
glib/gmacros.h:939:25: note: in definition of macro ‘G_LIKELY’
939 | #define G_LIKELY(expr) (expr)
| ^~~~
glib/gfileutils.c:1176:7: note: in expansion of macro ‘g_assert’
1176 | g_assert (s <= length);
| ^~~~~~~~
Related to issue #1735 (Get back to a -werror build)
Various different BSD systems use a different errno from `E_LOOP` (as
defined by POSIX and used on Linux) to indicate that a file is a symlink
when you try to `open()` it with `O_NOFOLLOW`.
Fix the code which detects this. This is a follow-up to #1302.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
This is used when creating the temporary file, or new file from scratch.
I wondered about also allowing the file owner and group to be set, but
that’s not as generally applicable — if your process is operating across
multiple user IDs then it likely has some fairly OS-specific
requirements and will need tighter control of its syscalls anyway.
(Eventually, support for setting the file owner and group atomically
could be added by writing out a file using `O_TMPFILE` so it’s not
addressable, and then linking it into the file system in place of the
old file using something like `renameat2(AT_EMPTY_PATH)` or `linkat()`.
That’s currently not possible without patching the kernel with
https://marc.info/?l=linux-fsdevel&m=152472898003523&w=2, as far as I
know at the moment.)
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1203
This moves `write_to_temp_file()` into `g_file_set_contents_full()` and
coalesces its handling of `do_fsync` with the `rename_file()` call. It
adds support for `G_FILE_SET_CONTENTS_DURABLE` and
`G_FILE_SET_CONTENTS_NONE` — previously only
`G_FILE_SET_CONTENTS_CONSISTENT | G_FILE_SET_CONTENTS_ONLY_EXISTING` was
supported.
In the case that `G_FILE_SET_CONTENTS_CONSISTENT |
G_FILE_SET_CONTENTS_DURABLE` is set, an additional `fsync()` is now done
on the directory after renaming the temporary file.
In the case that `G_FILE_SET_CONTENTS_ONLY_EXISTING` isn’t set, the
`fsync()` after writing the temporary file will always be done (unless
the file system guarantees it never needs to be done).
In the case that only `G_FILE_SET_CONTENTS_DURABLE` is set, the
destination file will be written to directly (using this mode is not
really advised).
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Fixes: #1302
This introduces no functional changes, just makes the code a bit more
modular and reusable.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1302
This is a new version of the g_file_set_contents() API which will allow
its safety to be controlled by some flags, allowing the user to choose
their preferred tradeoff between safety (`fsync()` calls) and speed.
Currently, the flags do nothing and the new API behaves like the old
API. This will change in the following commits.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Helps: #1302
`base` can be `-1` in some situations, which would lead to pointing
outside an allocation area if the sums were evaluated as `(file_name +
base) + 1` rather than `file_name + (base + 1)`.
I don’t see how this can practically cause an issue, as the arithmetic
is all finished before anything’s dereferenced, but let’s keep to the
letter of the C standard to avoid this coming up in code audits in
future.
Fix suggested by fablhx.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Closes: #2077
Commit 6f55306e04 unintendedly broke error handling for other
error conditions than ENOENT along the path, like EPERM. It wanted
to ignore ENOENT on all elements except the last in the path, but
in doing that it ignored any other error that might happen on the
last element.
https://gitlab.gnome.org/GNOME/glib/issues/1852
