6:16 p.m.
A detailed explanation of why this is needed can be found at
https://bugzilla.redhat.com/show_bug.cgi?id=1578741#c8
Andrea Bolognani (3):
vircommand: Ensure buffers are NULL-terminated
virfile: Report error in virFileWrapperFdFree()
iohelper: Don't include newlines in error messages
src/util/iohelper.c | 2 +-
src/util/vircommand.c | 2 ++
src/util/virfile.c | 6 +++++-
3 files changed, 8 insertions(+), 2 deletions(-)
--
2.20.1
6:16 p.m.
New subject: [libvirt] [PATCH 1/3] vircommand: Ensure buffers are NULL-terminated
The memory allocated by VIR_REALLOC_N() is uninitialized,
which means it's not possible to figure out whether any
output was produced at all after the fact.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/vircommand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index d965068369..6e9e56d0c0 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd)
outfd = cmd->outfd;
if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0)
ret = -1;
+ *cmd->outbuf[0] = '\0';
}
if (cmd->errbuf) {
errfd = cmd->errfd;
if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0)
ret = -1;
+ *cmd->errbuf[0] = '\0';
}
if (ret == -1)
goto cleanup;
--
2.20.1
6:16 p.m.
New subject: [libvirt] [PATCH 2/3] virfile: Report error in virFileWrapperFdFree()
Logging the error is fine and all, but getting the information
to the user directly is even better.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virfile.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 271bf5e796..30cad87df9 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -351,8 +351,12 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
if (!wfd)
return;
+ /* If the command used to process IO has produced errors, it's fair
+ * to assume those will be more relevant to the user than whatever
+ * eg. QEMU can figure out on its own, so it's okay if we end up
+ * discarding an existing error */
if (wfd->err_msg && *wfd->err_msg)
- VIR_WARN("iohelper reports: %s", wfd->err_msg);
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s", wfd->err_msg);
virCommandAbort(wfd->cmd);
--
2.20.1
6:16 p.m.
New subject: [libvirt] [PATCH 3/3] iohelper: Don't include newlines in error messages
The newline was pretty arbitrary, and we're better off
without it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/iohelper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index 1ff4a7b314..aed7ef3184 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -236,7 +236,7 @@ main(int argc, char **argv)
return 0;
error:
- fprintf(stderr, _("%s: failure with %s\n: %s"),
+ fprintf(stderr, _("%s: failure with %s: %s"),
program_name, path, virGetLastErrorMessage());
exit(EXIT_FAILURE);
}
--
2.20.1
6:23 p.m.
New subject: [libvirt] [PATCH 1/3] vircommand: Ensure buffers are NULL-terminated
On Tue, Feb 05, 2019 at 04:16:21PM +0100, Andrea Bolognani wrote:
The memory allocated by VIR_REALLOC_N() is uninitialized,
which means it's not possible to figure out whether any
output was produced at all after the fact.
I really wish we had never added the VIR_REALLOC_N function.
One of the best things about VIR_ALLOC/VIR_EXPAND/VIR_RESIZE
are that they remove all bugs related to use of uninitialized
memory. We really ought to try to eliminate use of VIR_REALLOC_N
in favour of the other safer functions throughout the code.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/vircommand.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index d965068369..6e9e56d0c0 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd)
outfd = cmd->outfd;
if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0)
ret = -1;
+ *cmd->outbuf[0] = '\0';
}
if (cmd->errbuf) {
errfd = cmd->errfd;
if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0)
ret = -1;
+ *cmd->errbuf[0] = '\0';
Here we don't really care about the original contents on outbuf/errbuf.
I'd probably go for making that explicit by replacing VIR_REALLOC_N
with VIR_FREE + VIR_ALLOC_N
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:52 p.m.
New subject: [libvirt] [PATCH 1/3] vircommand: Ensure buffers are NULL-terminated
On Tue, 2019-02-05 at 15:23 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 05, 2019 at 04:16:21PM +0100, Andrea Bolognani wrote:
> The memory allocated by VIR_REALLOC_N() is uninitialized,
> which means it's not possible to figure out whether any
> output was produced at all after the fact.
I really wish we had never added the VIR_REALLOC_N function.
One of the best things about VIR_ALLOC/VIR_EXPAND/VIR_RESIZE
are that they remove all bugs related to use of uninitialized
memory. We really ought to try to eliminate use of VIR_REALLOC_N
in favour of the other safer functions throughout the code.
That should be reasonably simple to do, since the functions we'd be
moving to offer more guarantees than the ones we'd be moving away
from. There are quite a few call sites, though :)
[...]
> @@ -2057,11 +2057,13 @@ virCommandProcessIO(virCommandPtr cmd)
> outfd = cmd->outfd;
> if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0)
> ret = -1;
> + *cmd->outbuf[0] = '\0';
> }
> if (cmd->errbuf) {
> errfd = cmd->errfd;
> if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0)
> ret = -1;
> + *cmd->errbuf[0] = '\0';
Here we don't really care about the original contents on outbuf/errbuf.
I'd probably go for making that explicit by replacing VIR_REALLOC_N
with VIR_FREE + VIR_ALLOC_N
Sure, can do.
--
Andrea Bolognani / Red Hat / Virtualization
2355
days inactive
2355
days old
5 comments
2 participants
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé