When popen or do_inotify_read fails, the file descriptor created by
mkstemp is never closed before returning. Add close(fd) to both
error paths to prevent file descriptor leaks.
Co-authored-by: Claude <noreply@anthropic.com>
Run this command across the source:
perl -pi.bak -e 's/(20[012][0-9])-20[12][012]/$1-2023/g' `git ls-files`
and remove changes to po{,-docs}/*.po{,t} (these will be regenerated
later when we run 'make dist').
GUESTFSD_EXT_CMD was used by OpenSUSE to track which external commands
are run by the daemon and package those commands into the appliance.
It is no longer used by recent SUSE builds, so remove it.
Thanks: Pino Toscano, Olaf Hering.
In every instance where we used the ‘cancel_stmt’ parameter of
these macros:
ABS_PATH
NEED_ROOT
the value was only ever ‘cancel_receive ()’ or empty. We only use
‘cancel_receive’ for FileIn functions, so replace it with a simple
flag for whether the current function is a FileIn function.
This also removes some incorrect comments about macros that cannot be
used with FileIn functions when in fact they can.
The previous code:
fcntl (fd, F_SETFL, O_NONBLOCK)
was technically incorrect, because it would have reset any
other flags on the file descriptor.
Thanks: Eric Blake
Run the following command over the source:
perl -pi.bak -e 's/(20[01][0-9])-2016/$1-2017/g' `git ls-files`
(Thanks Rich for the perl snippet, as used in past years.)
Declare most of the stringsbuf as CLEANUP_FREE_STRINGSBUF, so they are
freed completely on stack unwind: use take_stringsbuf() in return
places to take away from the stringsbuf its content, and remove all the
manual calls to free_stringslen (no more needed now).
This requires to not use free_stringslen anymore on failure in the
helper functions of stringsbuf, which now leave the content as-is (might
be still useful even on error).
This allows us to simplify the memory management of stringsbuf's, which
are not properly fully freed, fixing memory leaks in some error paths
(which were not calling free_stringslen).
The warning (see below) is fairly useless. This modification to the
code avoids it.
inotify.c: In function 'do_inotify_read':
inotify.c:219:32: error: logical 'or' of equal expressions [-Werror=logical-op]
if (errno == EWOULDBLOCK || errno == EAGAIN) /* End of list. */
^~
Since we as developers rarely test the case where some library is
statically not available, that side of the code was hardly tested,
except by unfortunate users in the field who often hit cases where
functions were missing or misdeclared. In fact, when making this
change I noticed several bugs like that.
Change it so that this code is autogenerated, and therefore always
correct and up to date.
Previous code which looked like this:
int
optgroup_acl_available (void)
{
return 0;
}
char * __attribute__((noreturn))
do_acl_get_file (const char *path, const char *acltype)
{
abort ();
}
/* etc */
is replaced by a single line:
OPTGROUP_ACL_NOT_AVAILABLE
guestfsd calls many different tools. Keeping track of all of them is
error prone. This patch introduces a new helper macro to put the command
string into its own ELF section:
GUESTFSD_EXT_CMD(C_variable, command_name);
This syntax makes it still possible to grep for used command names.
The actual usage of the collected list could be like this:
objcopy -j .guestfsd_ext_cmds -O binary daemon/guestfsd /dev/stdout |
tr '\0' '\n' | sort -u
The resulting output will be used to tell mkinitrd which programs to
copy into the initrd.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
RWMJ:
- Move str_vgchange at request of author.
- Fix snprintf call in daemon/debug.c
Previously a lot of daemon code used three variables (a string list,
'int size' and 'int alloc') to track growable strings buffers. This
commit implements a simple struct containing the same variables, but
using size_t instead of int:
struct stringsbuf {
char **argv;
size_t size;
size_t alloc;
};
Use it like this:
DECLARE_STRINGSBUF (ret);
//...
if (add_string (&ret, str) == -1)
return NULL;
//...
if (end_stringsbuf (&ret) == -1)
return NULL;
return ret.argv;
The Coverity error is this (which I think is wrong):
Error: TAINTED_SCALAR:
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:211: tainted_data_argument: Calling function "read" taints argument "inotify_buf".
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:232: var_assign_var: Assigning: "event" = "(struct inotify_event *)&inotify_buf[n]". Both are now tainted.
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:258: lower_bounds: Checking lower bounds of unsigned scalar "event->len" by "event->len > 0U".
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:272: var_assign_var: Compound assignment involving tainted variable "16UL + event->len" to variable "n" taints "n".
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:228: lower_bounds: Checking lower bounds of unsigned scalar "n" by "n < inotify_posn".
/builddir/build/BUILD/libguestfs-1.16.5/daemon/inotify.c:281: tainted_data: Using tainted variable "n" as an index into an array "inotify_buf".
Adding a sanity check of event->len is prudent.
Callers are supposed to use the availability API to check for
functions that may not be available in particular builds of
libguestfs. If they don't do this, currently they tend to get obscure
error messages, eg:
libguestfs: error: zerofree: /dev/vda1: zerofree: No such file or directory
This commit changes the error message to explain what callers ought to
be doing instead:
libguestfs: error: zerofree: feature 'zerofree' is not available in this
build of libguestfs. Read 'AVAILABILITY' in the guestfs(3) man page for
how to check for the availability of features.
This patch makes the stubs check for availability. The stub code
changes to:
static void
zerofree_stub (XDR *xdr_in)
{
[...]
/* The caller should have checked before calling this. */
if (! optgroup_zerofree_available ()) {
reply_with_error ("feature '%s' is not available in this\n"
"build of libguestfs. Read 'AVAILABILITY' in the guestfs(3) man page for\n"
"how to check for the availability of features.",
"zerofree");
goto done;
}
[...]
Although this doesn't matter for the ordinary (appliance) case, it
matters for the libguestfs live case. In that case it could cause the
guest to be exploited by a tmp/symlink attack.
As a previous, incorrect attempt to fix RHBZ#576879 we tried to
prevent the daemon from sending an error reply if the daemon had
cancelled the transfer. This is wrong: the daemon should send an
error reply in these cases.
A simple test case is this:
guestfish -N fs -m /dev/sda1 upload big-file /
(This fails because the target "/" is a directory, not a file.)
Prior to this commit, libguestfs would hang instead of printing an
error. With this commit, libguestfs prints an error.
What is happening is:
(1) Library is uploading
a file (2) In the middle of the long
upload, daemon detects an error.
Daemon cancels.
(3) Library detects cancel,
sends cancel chunk, then waits
for the error reply from the
daemon. (4) Daemon is supposed to send
an error reply message.
Because step (4) wasn't happening, uploads that failed like this would
hang in the library (waiting for the error message, while the daemon
was waiting for the next request).
This also adds a regression test.
This temporarily breaks the "both ends cancel" case (RHBZ#576879c5).
Therefore the test for that is disabled, and this is fixed in the next
patch in the series.
This partially reverts commit dc706a639e.
During a FileIn command (eg. upload, tar-in) if both sides
experience errors, then both sides could send cancel messages,
the result being lost synchronization.
The reason for the lost synch was because the daemon was ignoring
this case and sending an error message back which the library side
(which had cancelled) was not expecting.
Fix this by checking in the daemon for the case where the library
also cancels during daemon cancellation, and not sending an error
messages.
This also includes an enhanced regression test which checks for this
case.
This extends the original fix in
commit 5922d7084d.
More details can be found here:
https://bugzilla.redhat.com/show_bug.cgi?id=576879#c5
Modify the generator so that it can correctly handle early
cancellation for Pathname|Device|.. parameters. This fixes
the upload command, but consequently we need to fix the
parameters for tar_in and t?z_in commands. This should also
mean that 'win:' can now be used as the second argument of
tar_in and t?z_in commands in guestfish, whereas previously
this wouldn't have worked.
Adds a regression test for the original problem.
The RPC stubs already prefix the command name to error messages.
The daemon doesn't have to do this. As a (small) benefit this also
makes the daemon slightly smaller.
Code in the daemon such as:
if (argv[0] == NULL) {
reply_with_error ("passed an empty list");
return NULL;
}
now results in error messages like this:
><fs> command ""
libguestfs: error: command: passed an empty list
(whereas previously you would have seen ..command: command:..)
The current groups are defined very conservatively using the
following criteria:
(a) Would be impossible to implement on Windows because of
sheer architectural differences (eg: mknod).
(b) Already optional (augeas, inotify).
(c) Not currently optional but not implemented on older RHEL and
Debian releases (ntfs-3g.probe, scrub, zerofree).
The optional groups I've defined according to these criteria are:
. augeas
. inotify
. linuxfsuuid
. linuxmodules
. linuxxattrs
. lvm2
. mknod
. ntfs3g
. scrub
. selinux
. zerofree
(Note that these choices don't prevent us from adding more
optional groups in future. On the other hand to avoid breaking
ABIs we would not wish to change the above groups).
The rest of this large commit is really just implementation:
Each optional function is classified using Optional "group"
flag in the generator.
The daemon has to implement a function
int optgroup_<name>_available (void);
for each optional group. Some of these functions are fixed at
compile time, and some do simple run-time tests.
The do_available implementation in the daemon looks up the correct
function in a table and runs it.
We document the optional groups in the guestfs(3) man page.
Also: I added a NOT_AVAILABLE macro in order to unify all the
existing places where we had a message equivalent to
"function __func__ is not available".
inotify: Make this optional on platforms that don't have this interface.
mknod, mkfifo etc.: Make these optional on non-Unix platforms.
readdir: If d_type field is missing on the platform, set the corresponding
field to 'u'.
stat: st_blocks and st_blksize are missing on non-Unix platforms, so
set these fields to -1 in the corresponding structures.
Nearly every file-related function in daemons/*.c is affected:
Remove this pair of statements from each affected do_* function:
- NEED_ROOT (return -1);
- ABS_PATH (dir, return -1);
and change the type of the corresponding parameter to "const char *".
* src/generator.ml: Emit NEED_ROOT just once, even when there are two or
more Pathname args.
Avoid "comparison between signed and unsigned integer expressions"
warnings. If it's at all hard or risky to avoid this type of warning,
then it's not worthwhile. Here, it's easy and safe.
* daemon/inotify.c (inotify_posn): Declare local to be of unsigned type.
(do_inotify_read, do_inotify_files): Likewise.
