[libvirt] [PATCHv2] virsh: avoid uninitialized memory usage
Detected by valgrind. * tools/virsh.c (cmdBlockPull): fix uninitialized memory usage. * How to reproduce? $ qemu-img create /var/lib/libvirt/images/test 1M $ cat > /tmp/test.xml <<EOF <domain type='qemu'> <name>test</name> <memory>219200</memory> <vcpu>1</vcpu> <os> <type arch='x86_64'>hvm</type> <boot dev='hd'/> </os> <devices> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test'/> <target dev='vda' bus='virtio'/> </disk> <input type='mouse' bus='ps2'/> <graphics type='spice' autoport='yes' listen='0.0.0.0'/> </devices> </domain> EOF $ virsh define /tmp/test.xml $ valgrind -v virsh blockpull test /var/lib/libvirt/images/test --wait actual result: ==10906== 1 errors in context 1 of 1: ==10906== Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s) ==10906== at 0x39CF80F5BE: __libc_sigaction (sigaction.c:67) ==10906== by 0x43016C: cmdBlockPull (virsh.c:7638) ==10906== by 0x4150D4: vshCommandRun (virsh.c:18574) ==10906== by 0x425E73: main (virsh.c:20178) ==10906== Address 0x7fefffae8 is on thread 1's stack Signed-off-by: Alex Jia <ajia@redhat.com> --- tools/virsh.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..4e4ca57 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7634,6 +7634,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) intCaught = 0; sig_action.sa_sigaction = vshCatchInt; + sig_action.sa_flags = 0; sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action); -- 1.7.1
At 04/19/2012 04:51 PM, Alex Jia Wrote:
Detected by valgrind.
* tools/virsh.c (cmdBlockPull): fix uninitialized memory usage.
* How to reproduce? $ qemu-img create /var/lib/libvirt/images/test 1M $ cat > /tmp/test.xml <<EOF <domain type='qemu'> <name>test</name> <memory>219200</memory> <vcpu>1</vcpu> <os> <type arch='x86_64'>hvm</type> <boot dev='hd'/> </os> <devices> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test'/> <target dev='vda' bus='virtio'/> </disk> <input type='mouse' bus='ps2'/> <graphics type='spice' autoport='yes' listen='0.0.0.0'/> </devices> </domain> EOF $ virsh define /tmp/test.xml $ valgrind -v virsh blockpull test /var/lib/libvirt/images/test --wait
actual result:
==10906== 1 errors in context 1 of 1: ==10906== Syscall param rt_sigaction(act->sa_flags) points to uninitialised byte(s) ==10906== at 0x39CF80F5BE: __libc_sigaction (sigaction.c:67) ==10906== by 0x43016C: cmdBlockPull (virsh.c:7638) ==10906== by 0x4150D4: vshCommandRun (virsh.c:18574) ==10906== by 0x425E73: main (virsh.c:20178) ==10906== Address 0x7fefffae8 is on thread 1's stack
Signed-off-by: Alex Jia <ajia@redhat.com> --- tools/virsh.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 95ed7bc..4e4ca57 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -7634,6 +7634,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
intCaught = 0; sig_action.sa_sigaction = vshCatchInt; + sig_action.sa_flags = 0; sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action);
ACK
On 04/19/2012 02:56 AM, Wen Congyang wrote:
At 04/19/2012 04:51 PM, Alex Jia Wrote:
Detected by valgrind.
* tools/virsh.c (cmdBlockPull): fix uninitialized memory usage.
+++ b/tools/virsh.c @@ -7634,6 +7634,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
intCaught = 0; sig_action.sa_sigaction = vshCatchInt; + sig_action.sa_flags = 0; sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action);
ACK
NACK. vshCatchInt is a 3-arg function, and therefore sig_action.sa_flags must include at least SA_SIGINFO. I inadvertently missed a line when copying code from vshWatchJob(); I'll push the obvious v3 patch shortly. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At 04/20/2012 04:29 AM, Eric Blake Wrote:
On 04/19/2012 02:56 AM, Wen Congyang wrote:
At 04/19/2012 04:51 PM, Alex Jia Wrote:
Detected by valgrind.
* tools/virsh.c (cmdBlockPull): fix uninitialized memory usage.
+++ b/tools/virsh.c @@ -7634,6 +7634,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
intCaught = 0; sig_action.sa_sigaction = vshCatchInt; + sig_action.sa_flags = 0; sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action);
ACK
NACK. vshCatchInt is a 3-arg function, and therefore sig_action.sa_flags must include at least SA_SIGINFO. I inadvertently missed a line when copying code from vshWatchJob(); I'll push the obvious v3 patch shortly.
Sorry, I forgot this. I search sig_action(), and found the same problem in the function virNetServerNew(): ==================== memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_IGN; sigaction(SIGPIPE, &sig_action, NULL); /* * catch fatal errors to dump a log, also hook to USR2 for dynamic * debugging purposes or testing */ sig_action.sa_sigaction = virNetServerFatalSignal; sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); ==================== We set sig_action.sa_flags to 0 here. And in virsh.c, I found: ==================== /* Gnulib doesn't guarantee SA_SIGINFO support. */ #ifndef SA_SIGINFO # define SA_SIGINFO 0 #endif ==================== Is it safe to use sig_action.sa_sigaction when SA_SIGINFO is not set? I think it is OK if we access siginfo_t if SA_SIGINFO is not 0. But in the function virNetServerAddSignalHandler(): ==================== memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_sigaction = virNetServerSignalHandler; #ifdef SA_SIGINFO sig_action.sa_flags = SA_SIGINFO; #endif sigemptyset(&sig_action.sa_mask); sigaction(signum, &sig_action, &sigdata->oldaction); ==================== virNetServerSignalHandler() will access siginfo_t without any check.
On 04/19/2012 06:43 PM, Wen Congyang wrote:
I search sig_action(), and found the same problem in the function virNetServerNew(): ==================== memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_IGN; sigaction(SIGPIPE, &sig_action, NULL);
/* * catch fatal errors to dump a log, also hook to USR2 for dynamic * debugging purposes or testing */ sig_action.sa_sigaction = virNetServerFatalSignal; sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); ==================== We set sig_action.sa_flags to 0 here.
Thanks for searching. This is a bug in theory: POSIX requires that you use sa_sigaction if sa_flags includes SA_SIGINFO, and that you use sa_handler otherwise, and while it permits sa_handler and sa_sigaction to overlap, it does not require this. Thankfully, in practice, I know of no platform where sa_sigaction and sa_handler do not overlap; furthermore, I don't know of any commonly used ABIs where casting a 1-arg function pointer and calling it with 3 arguments breaks (the extra 2 args are ignored by the callee); likewise, commonly used ABIs state that if you cast a 3-arg function pointer and call it with only 1 argument, you have predictable behavior so long as you limit yourself to the first argument (but you _are_ likely to misbehave if you try to access the other two arguments). Again, practice says it will work, but theory says you can't rely on it to work, so we should fix it. Do you want to prepare the patch, or shall I?
And in virsh.c, I found: ==================== /* Gnulib doesn't guarantee SA_SIGINFO support. */ #ifndef SA_SIGINFO # define SA_SIGINFO 0 #endif ====================
Is it safe to use sig_action.sa_sigaction when SA_SIGINFO is not set? I think it is OK if we access siginfo_t if SA_SIGINFO is not 0.
Gnulib doesn't guarante SA_SIGINFO, but it _does_ guarantee that for all platforms where SA_SIGINFO is not defined, then sa_sigaction overlaps with sa_handler, and that such platforms have a calling convention . Therefore, on those platforms, we end up installing a sa_handler, and given my above statements about practice, as long as we don't access the second or third argument, we won't trigger any odd behaviors.
But in the function virNetServerAddSignalHandler(): ==================== memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_sigaction = virNetServerSignalHandler; #ifdef SA_SIGINFO sig_action.sa_flags = SA_SIGINFO; #endif sigemptyset(&sig_action.sa_mask);
sigaction(signum, &sig_action, &sigdata->oldaction); ==================== virNetServerSignalHandler() will access siginfo_t without any check.
Yep, definite bug for mingw. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
POSIX requires that we use sa_sigaction if sa_flags includes SA_SIGINFO, and that we use sa_handler otherwise. But we still use sa_sigaction when SA_SIGINFO is not defined. Practice says it will work, but theory says we can't rely on it to work. --- src/rpc/virnetserver.c | 25 +++++++++++++++++++++++-- tools/virsh.c | 35 ++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 3965fc2..131ecb4 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -270,8 +270,12 @@ error: } +#ifdef SA_SIGINFO static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void virNetServerFatalSignal(int sig) +#endif { struct sigaction sig_action; int origerrno; @@ -286,6 +290,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED #ifdef SIGUSR2 if (sig != SIGUSR2) { #endif + memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_DFL; sigaction(sig, &sig_action, NULL); raise(sig); @@ -362,7 +367,12 @@ virNetServerPtr virNetServerNew(size_t min_workers, * catch fatal errors to dump a log, also hook to USR2 for dynamic * debugging purposes or testing */ +#ifdef SA_SIGINFO sig_action.sa_sigaction = virNetServerFatalSignal; + sig_action.sa_flags = SA_SIGINFO; +#else + sig_action.sa_handler = virNetServerFatalSignal; +#endif sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); sigaction(SIGILL, &sig_action, NULL); @@ -420,17 +430,28 @@ static sig_atomic_t sigErrors = 0; static int sigLastErrno = 0; static int sigWrite = -1; +#ifdef SA_SIGINFO static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, void* context ATTRIBUTE_UNUSED) +#else +static void virNetServerSignalHandler(int sig) +#endif { int origerrno; int r; + siginfo_t temp_siginfo; + +#ifdef SA_SIGINFO + memcpy(&temp_siginfo, siginfo, sizeof(temp_siginfo)); +#else + memset(&temp_siginfo, 0, sizeof(temp_siginfo)); +#endif /* set the sig num in the struct */ - siginfo->si_signo = sig; + temp_siginfo.si_signo = sig; origerrno = errno; - r = safewrite(sigWrite, siginfo, sizeof(*siginfo)); + r = safewrite(sigWrite, &temp_siginfo, sizeof(temp_siginfo)); if (r == -1) { sigErrors++; sigLastErrno = errno; diff --git a/tools/virsh.c b/tools/virsh.c index 6168a13..cb64e81 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -579,9 +579,13 @@ out: static volatile sig_atomic_t intCaught = 0; +#ifdef SA_SIGINFO static void vshCatchInt(int sig ATTRIBUTE_UNUSED, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void vshCatchInt(int sig ATTRIBUTE_UNUSED) +#endif { intCaught = 1; } @@ -591,23 +595,25 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED, */ static int disconnected = 0; /* we may have been disconnected */ -/* Gnulib doesn't guarantee SA_SIGINFO support. */ -#ifndef SA_SIGINFO -# define SA_SIGINFO 0 -#endif - /* * vshCatchDisconnect: * * We get here when a SIGPIPE is being raised, we can't do much in the * handler, just save the fact it was raised */ +#ifdef SA_SIGINFO static void vshCatchDisconnect(int sig, siginfo_t *siginfo, void *context ATTRIBUTE_UNUSED) { - if (sig == SIGPIPE || - (SA_SIGINFO && siginfo->si_signo == SIGPIPE)) + if (sig == SIGPIPE || siginfo->si_signo == SIGPIPE) + disconnected++; +} +#else +static void vshCatchDisconnect(int sig) +{ + if (sig == SIGPIPE) disconnected++; } +#endif /* * vshSetupSignals: @@ -619,8 +625,13 @@ static void vshSetupSignals(void) { struct sigaction sig_action; +#ifdef SA_SIGINFO sig_action.sa_sigaction = vshCatchDisconnect; sig_action.sa_flags = SA_SIGINFO; +#else + sig_action.sa_handler = vshCatchDisconnect; + sig_action.sa_flags = 0; +#endif sigemptyset(&sig_action.sa_mask); sigaction(SIGPIPE, &sig_action, NULL); @@ -7254,8 +7265,13 @@ vshWatchJob(vshControl *ctl, sigaddset(&sigmask, SIGINT); intCaught = 0; +#ifdef SA_SIGINFO sig_action.sa_sigaction = vshCatchInt; sig_action.sa_flags = SA_SIGINFO; +#else + sig_action.sa_handler = vshCatchInt; + sig_action.sa_flags = 0; +#endif sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action); @@ -7631,8 +7647,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) sigaddset(&sigmask, SIGINT); intCaught = 0; +#ifdef SA_SIGINFO sig_action.sa_sigaction = vshCatchInt; sig_action.sa_flags = SA_SIGINFO; +#else + sig_action.sa_handler = vshCatchInt; + sig_action.sa_flags = 0; +#endif sigemptyset(&sig_action.sa_mask); sigaction(SIGINT, &sig_action, &old_sig_action); -- 1.7.1
On 04/19/2012 09:10 PM, Wen Congyang wrote:
POSIX requires that we use sa_sigaction if sa_flags includes SA_SIGINFO, and that we use sa_handler otherwise. But we still use sa_sigaction when SA_SIGINFO is not defined. Practice says it will work, but theory says we can't rely on it to work.
Not quite what I said. Gnulib guarantees that sa_sigaction will work instead of sa_handler, _for the platforms where SA_SIGINFO is not defined_, and provided that your handler does not access the 2nd or 3rd argument in those situations. That is, gnulib gives us some guarantees that POSIX does not, and that allows us to write simpler code that assumes SA_SIGINFO just works everywhere, as long as the handler is careful with the last two arguments. For all platforms where SA_SIGINFO is defined, using just sa_sigaction is fine. tools/virsh.c is therefore correct, and only src/rpc/virnetserver.c has a bug.
--- src/rpc/virnetserver.c | 25 +++++++++++++++++++++++-- tools/virsh.c | 35 ++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 3965fc2..131ecb4 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -270,8 +270,12 @@ error: }
+#ifdef SA_SIGINFO static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void virNetServerFatalSignal(int sig) +#endif
Yuck. I'm happy with a single three-arg definition as long as we follow the gnulib rules about using sa_sigaction and SA_SIGINFO as a pair, rather than adding ugly #ifdefs.
{ struct sigaction sig_action; int origerrno; @@ -286,6 +290,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED #ifdef SIGUSR2 if (sig != SIGUSR2) { #endif + memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_DFL; sigaction(sig, &sig_action, NULL);
Good catch - that memset (or an explicit setting of sig_action.sa_flags = 0) is required; just because Linux behaves sanely for sa_handler==SIG_DFL regardless of uninitialized sa_flags does not mean all platforms do likewise.
raise(sig); @@ -362,7 +367,12 @@ virNetServerPtr virNetServerNew(size_t min_workers, * catch fatal errors to dump a log, also hook to USR2 for dynamic * debugging purposes or testing */ +#ifdef SA_SIGINFO sig_action.sa_sigaction = virNetServerFatalSignal; + sig_action.sa_flags = SA_SIGINFO;
This one line is important,
+#else + sig_action.sa_handler = virNetServerFatalSignal; +#endif
this #ifdef stuff is just gross. We should instead copy virsh.c and #define SA_SIGINFO 0 if it is not already defined.
sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); sigaction(SIGILL, &sig_action, NULL); @@ -420,17 +430,28 @@ static sig_atomic_t sigErrors = 0; static int sigLastErrno = 0; static int sigWrite = -1;
+#ifdef SA_SIGINFO static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, void* context ATTRIBUTE_UNUSED) +#else +static void virNetServerSignalHandler(int sig) +#endif
Not sure I like the #ifdef here,
{ int origerrno; int r; + siginfo_t temp_siginfo; + +#ifdef SA_SIGINFO + memcpy(&temp_siginfo, siginfo, sizeof(temp_siginfo)); +#else + memset(&temp_siginfo, 0, sizeof(temp_siginfo)); +#endif
Here, if you start the file with: #ifndef SA_SIGINFO # define SA_SIGINFO 0 #endif then here, you can avoid the in-function #ifdef with: if (SA_SIGINFO) memcpy(&tmp_siginfo, siginfo, sizeof(temp_siginfo)); else memset(&temp_siginfo, 0, sizeof(temp_siginfo));
/* set the sig num in the struct */ - siginfo->si_signo = sig; + temp_siginfo.si_signo = sig;
origerrno = errno; - r = safewrite(sigWrite, siginfo, sizeof(*siginfo)); + r = safewrite(sigWrite, &temp_siginfo, sizeof(temp_siginfo));
This change makes sense (the caller won't get much more than si_siginfo out of their virNetServerSignalFunc callback, but at least we are no longer passing random memory to the callback on mingw).
+++ b/tools/virsh.c @@ -579,9 +579,13 @@ out:
static volatile sig_atomic_t intCaught = 0;
+#ifdef SA_SIGINFO static void vshCatchInt(int sig ATTRIBUTE_UNUSED, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void vshCatchInt(int sig ATTRIBUTE_UNUSED) +#endif
Not needed.
{ intCaught = 1; } @@ -591,23 +595,25 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED, */ static int disconnected = 0; /* we may have been disconnected */
-/* Gnulib doesn't guarantee SA_SIGINFO support. */ -#ifndef SA_SIGINFO -# define SA_SIGINFO 0 -#endif
Keep this, and copy it to virnetserver.c (or even float it to a common location, like internal.h). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At 04/20/2012 11:26 AM, Eric Blake Wrote:
On 04/19/2012 09:10 PM, Wen Congyang wrote:
POSIX requires that we use sa_sigaction if sa_flags includes SA_SIGINFO, and that we use sa_handler otherwise. But we still use sa_sigaction when SA_SIGINFO is not defined. Practice says it will work, but theory says we can't rely on it to work.
Not quite what I said. Gnulib guarantees that sa_sigaction will work instead of sa_handler, _for the platforms where SA_SIGINFO is not defined_, and provided that your handler does not access the 2nd or 3rd argument in those situations. That is, gnulib gives us some guarantees that POSIX does not, and that allows us to write simpler code that assumes SA_SIGINFO just works everywhere, as long as the handler is careful with the last two arguments. For all platforms where SA_SIGINFO is defined, using just sa_sigaction is fine.
Sorry for my misunderstanding. I will update this patch. Thanks Wen Congyang
tools/virsh.c is therefore correct, and only src/rpc/virnetserver.c has a bug.
--- src/rpc/virnetserver.c | 25 +++++++++++++++++++++++-- tools/virsh.c | 35 ++++++++++++++++++++++++++++------- 2 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 3965fc2..131ecb4 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -270,8 +270,12 @@ error: }
+#ifdef SA_SIGINFO static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void virNetServerFatalSignal(int sig) +#endif
Yuck. I'm happy with a single three-arg definition as long as we follow the gnulib rules about using sa_sigaction and SA_SIGINFO as a pair, rather than adding ugly #ifdefs.
{ struct sigaction sig_action; int origerrno; @@ -286,6 +290,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED #ifdef SIGUSR2 if (sig != SIGUSR2) { #endif + memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_DFL; sigaction(sig, &sig_action, NULL);
Good catch - that memset (or an explicit setting of sig_action.sa_flags = 0) is required; just because Linux behaves sanely for sa_handler==SIG_DFL regardless of uninitialized sa_flags does not mean all platforms do likewise.
raise(sig); @@ -362,7 +367,12 @@ virNetServerPtr virNetServerNew(size_t min_workers, * catch fatal errors to dump a log, also hook to USR2 for dynamic * debugging purposes or testing */ +#ifdef SA_SIGINFO sig_action.sa_sigaction = virNetServerFatalSignal; + sig_action.sa_flags = SA_SIGINFO;
This one line is important,
+#else + sig_action.sa_handler = virNetServerFatalSignal; +#endif
this #ifdef stuff is just gross. We should instead copy virsh.c and #define SA_SIGINFO 0 if it is not already defined.
sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); sigaction(SIGILL, &sig_action, NULL); @@ -420,17 +430,28 @@ static sig_atomic_t sigErrors = 0; static int sigLastErrno = 0; static int sigWrite = -1;
+#ifdef SA_SIGINFO static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, void* context ATTRIBUTE_UNUSED) +#else +static void virNetServerSignalHandler(int sig) +#endif
Not sure I like the #ifdef here,
{ int origerrno; int r; + siginfo_t temp_siginfo; + +#ifdef SA_SIGINFO + memcpy(&temp_siginfo, siginfo, sizeof(temp_siginfo)); +#else + memset(&temp_siginfo, 0, sizeof(temp_siginfo)); +#endif
Here, if you start the file with:
#ifndef SA_SIGINFO # define SA_SIGINFO 0 #endif
then here, you can avoid the in-function #ifdef with:
if (SA_SIGINFO) memcpy(&tmp_siginfo, siginfo, sizeof(temp_siginfo)); else memset(&temp_siginfo, 0, sizeof(temp_siginfo));
/* set the sig num in the struct */ - siginfo->si_signo = sig; + temp_siginfo.si_signo = sig;
origerrno = errno; - r = safewrite(sigWrite, siginfo, sizeof(*siginfo)); + r = safewrite(sigWrite, &temp_siginfo, sizeof(temp_siginfo));
This change makes sense (the caller won't get much more than si_siginfo out of their virNetServerSignalFunc callback, but at least we are no longer passing random memory to the callback on mingw).
+++ b/tools/virsh.c @@ -579,9 +579,13 @@ out:
static volatile sig_atomic_t intCaught = 0;
+#ifdef SA_SIGINFO static void vshCatchInt(int sig ATTRIBUTE_UNUSED, siginfo_t *siginfo ATTRIBUTE_UNUSED, void *context ATTRIBUTE_UNUSED) +#else +static void vshCatchInt(int sig ATTRIBUTE_UNUSED) +#endif
Not needed.
{ intCaught = 1; } @@ -591,23 +595,25 @@ static void vshCatchInt(int sig ATTRIBUTE_UNUSED, */ static int disconnected = 0; /* we may have been disconnected */
-/* Gnulib doesn't guarantee SA_SIGINFO support. */ -#ifndef SA_SIGINFO -# define SA_SIGINFO 0 -#endif
Keep this, and copy it to virnetserver.c (or even float it to a common location, like internal.h).
POSIX says that sa_sigaction is only safe to use if sa_flags includes SA_SIGINFO; conversely, sa_handler is only safe to use when flags excludes that bit. Gnulib doesn't guarantee an implementation of SA_SIGINFO, but does guarantee that if SA_SIGINFO is undefined, we can safely define it to 0 as long as we don't dereference the 2nd or 3rd argument of any handler otherwise registered via sa_sigaction. Based on a report by Wen Congyang. * src/rpc/virnetserver.c (SA_SIGINFO): Stub for mingw. (virNetServerSignalHandler): Avoid bogus dereference. (virNetServerFatalSignal, virNetServerNew): Set flags properly. --- v2: Simplify according to what gnulib gives us for mingw. src/rpc/virnetserver.c | 28 ++++++++++++++++++++-------- 1 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 3965fc2..3f3989e 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1,7 +1,7 @@ /* * virnetserver.c: generic network RPC server * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -40,6 +40,10 @@ # include "virnetservermdns.h" #endif +#ifndef SA_SIGINFO +# define SA_SIGINFO 0 +#endif + #define VIR_FROM_THIS VIR_FROM_RPC #define virNetError(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ @@ -270,8 +274,9 @@ error: } -static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, - void *context ATTRIBUTE_UNUSED) +static void +virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, + void *context ATTRIBUTE_UNUSED) { struct sigaction sig_action; int origerrno; @@ -286,6 +291,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED #ifdef SIGUSR2 if (sig != SIGUSR2) { #endif + memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_DFL; sigaction(sig, &sig_action, NULL); raise(sig); @@ -363,6 +369,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, * debugging purposes or testing */ sig_action.sa_sigaction = virNetServerFatalSignal; + sig_action.sa_flags = SA_SIGINFO; sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); sigaction(SIGILL, &sig_action, NULL); @@ -420,17 +427,24 @@ static sig_atomic_t sigErrors = 0; static int sigLastErrno = 0; static int sigWrite = -1; -static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, - void* context ATTRIBUTE_UNUSED) +static void +virNetServerSignalHandler(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED, + void* context ATTRIBUTE_UNUSED) { int origerrno; int r; + siginfo_t tmp; + + if (SA_SIGINFO) + tmp = *siginfo; + else + memset(&tmp, 0, sizeof(tmp)); /* set the sig num in the struct */ - siginfo->si_signo = sig; + tmp->si_signo = sig; origerrno = errno; - r = safewrite(sigWrite, siginfo, sizeof(*siginfo)); + r = safewrite(sigWrite, &tmp, sizeof(tmp)); if (r == -1) { sigErrors++; sigLastErrno = errno; @@ -533,9 +547,7 @@ int virNetServerAddSignalHandler(virNetServerPtr srv, memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_sigaction = virNetServerSignalHandler; -#ifdef SA_SIGINFO sig_action.sa_flags = SA_SIGINFO; -#endif sigemptyset(&sig_action.sa_mask); sigaction(signum, &sig_action, &sigdata->oldaction); -- 1.7.7.6
On 04/19/2012 09:45 PM, Eric Blake wrote:
POSIX says that sa_sigaction is only safe to use if sa_flags includes SA_SIGINFO; conversely, sa_handler is only safe to use when flags excludes that bit. Gnulib doesn't guarantee an implementation of SA_SIGINFO, but does guarantee that if SA_SIGINFO is undefined, we can safely define it to 0 as long as we don't dereference the 2nd or 3rd argument of any handler otherwise registered via sa_sigaction.
Based on a report by Wen Congyang.
* src/rpc/virnetserver.c (SA_SIGINFO): Stub for mingw. (virNetServerSignalHandler): Avoid bogus dereference. (virNetServerFatalSignal, virNetServerNew): Set flags properly. ---
+static void +virNetServerSignalHandler(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED, + void* context ATTRIBUTE_UNUSED) { int origerrno; int r; + siginfo_t tmp; + + if (SA_SIGINFO) + tmp = *siginfo; + else + memset(&tmp, 0, sizeof(tmp));
/* set the sig num in the struct */ - siginfo->si_signo = sig; + tmp->si_signo = sig;
I typo'd that (serves me right for sending before one last compile test); it should be tmp.si_signo. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
At 04/20/2012 11:45 AM, Eric Blake Wrote:
POSIX says that sa_sigaction is only safe to use if sa_flags includes SA_SIGINFO; conversely, sa_handler is only safe to use when flags excludes that bit. Gnulib doesn't guarantee an implementation of SA_SIGINFO, but does guarantee that if SA_SIGINFO is undefined, we can safely define it to 0 as long as we don't dereference the 2nd or 3rd argument of any handler otherwise registered via sa_sigaction.
Based on a report by Wen Congyang.
* src/rpc/virnetserver.c (SA_SIGINFO): Stub for mingw. (virNetServerSignalHandler): Avoid bogus dereference. (virNetServerFatalSignal, virNetServerNew): Set flags properly. ---
v2: Simplify according to what gnulib gives us for mingw.
src/rpc/virnetserver.c | 28 ++++++++++++++++++++-------- 1 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 3965fc2..3f3989e 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -1,7 +1,7 @@ /* * virnetserver.c: generic network RPC server * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -40,6 +40,10 @@ # include "virnetservermdns.h" #endif
+#ifndef SA_SIGINFO +# define SA_SIGINFO 0 +#endif + #define VIR_FROM_THIS VIR_FROM_RPC #define virNetError(code, ...) \ virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ @@ -270,8 +274,9 @@ error: }
-static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, - void *context ATTRIBUTE_UNUSED) +static void +virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED, + void *context ATTRIBUTE_UNUSED) { struct sigaction sig_action; int origerrno; @@ -286,6 +291,7 @@ static void virNetServerFatalSignal(int sig, siginfo_t *siginfo ATTRIBUTE_UNUSED #ifdef SIGUSR2 if (sig != SIGUSR2) { #endif + memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_handler = SIG_DFL; sigaction(sig, &sig_action, NULL); raise(sig); @@ -363,6 +369,7 @@ virNetServerPtr virNetServerNew(size_t min_workers, * debugging purposes or testing */ sig_action.sa_sigaction = virNetServerFatalSignal; + sig_action.sa_flags = SA_SIGINFO; sigaction(SIGFPE, &sig_action, NULL); sigaction(SIGSEGV, &sig_action, NULL); sigaction(SIGILL, &sig_action, NULL); @@ -420,17 +427,24 @@ static sig_atomic_t sigErrors = 0; static int sigLastErrno = 0; static int sigWrite = -1;
-static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, - void* context ATTRIBUTE_UNUSED) +static void +virNetServerSignalHandler(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED,
Why siginfo has the attribute ATTRIBUTE_UNUSED?
+ void* context ATTRIBUTE_UNUSED) { int origerrno; int r; + siginfo_t tmp; + + if (SA_SIGINFO) + tmp = *siginfo; + else + memset(&tmp, 0, sizeof(tmp));
/* set the sig num in the struct */ - siginfo->si_signo = sig; + tmp->si_signo = sig;
s/->/./
origerrno = errno; - r = safewrite(sigWrite, siginfo, sizeof(*siginfo)); + r = safewrite(sigWrite, &tmp, sizeof(tmp)); if (r == -1) { sigErrors++; sigLastErrno = errno; @@ -533,9 +547,7 @@ int virNetServerAddSignalHandler(virNetServerPtr srv,
memset(&sig_action, 0, sizeof(sig_action)); sig_action.sa_sigaction = virNetServerSignalHandler; -#ifdef SA_SIGINFO sig_action.sa_flags = SA_SIGINFO; -#endif sigemptyset(&sig_action.sa_mask);
sigaction(signum, &sig_action, &sigdata->oldaction);
ACK with nit fixed.
On 04/19/2012 09:55 PM, Wen Congyang wrote:
At 04/20/2012 11:45 AM, Eric Blake Wrote:
POSIX says that sa_sigaction is only safe to use if sa_flags includes SA_SIGINFO; conversely, sa_handler is only safe to use when flags excludes that bit. Gnulib doesn't guarantee an implementation of SA_SIGINFO, but does guarantee that if SA_SIGINFO is undefined, we can safely define it to 0 as long as we don't dereference the 2nd or 3rd argument of any handler otherwise registered via sa_sigaction.
Based on a report by Wen Congyang.
-static void virNetServerSignalHandler(int sig, siginfo_t * siginfo, - void* context ATTRIBUTE_UNUSED) +static void +virNetServerSignalHandler(int sig, siginfo_t * siginfo ATTRIBUTE_UNUSED,
Why siginfo has the attribute ATTRIBUTE_UNUSED?
I guess I copied too much from other locations; I've removed the attribute (on mingw, the variable is still considered 'used' even though its only use is in dead code).
ACK with nit fixed.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Alex Jia -
Eric Blake -
Wen Congyang