3:51 a.m.
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(a)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
3:56 a.m.
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(a)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
3:29 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:43 p.m.
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.
8:01 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:10 p.m.
New subject: [libvirt] [PATCH] use sa_handler instead of sa_sigaction when SA_SIGINFO is not defined
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
10:26 p.m.
New subject: [libvirt] [PATCH] use sa_handler instead of sa_sigaction when SA_SIGINFO is not defined
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:35 p.m.
New subject: [libvirt] [PATCH] use sa_handler instead of sa_sigaction when SA_SIGINFO is not defined
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).
10:45 p.m.
New subject: [libvirt] [PATCHv2] virnetserver: handle sigaction correctly
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
10:50 p.m.
New subject: [libvirt] [PATCHv2] virnetserver: handle sigaction correctly
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:55 p.m.
New subject: [libvirt] [PATCHv2] virnetserver: handle sigaction correctly
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.
11:08 p.m.
New subject: [libvirt] [PATCHv2] virnetserver: handle sigaction correctly
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4634
days inactive
4635
days old
11 comments
3 participants
participants (3)
-
Alex Jia -
Eric Blake -
Wen Congyang