Add sanity timeouts #142

Merged
merged 9 commits into from Sep 14, 2016

Conversation

Projects
None yet
2 participants
Collaborator

zyga commented Sep 14, 2016

This patch adds library functions to ensure that ensures a system call
such as flock() doesn't block for more than a given "sanity timeout"
value. This is meant to guard against bugs in the code that might
manifest as hanging process, keeping a flock-based lock alive, that will
never wake up.

The timer is implemented using SIGARLM and alarm(). The intent is to
simply wake up the system call and detect a flag being set by the signal
handler. This relies on using a signal handler that is not restarting
system calls.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Add sanity timeouts
This patch adds library functions to ensure that ensures a system call
such as flock() doesn't block for more than a given "sanity timeout"
value. This is meant to guard against bugs in the code that might
manifest as hanging process, keeping a flock-based lock alive, that will
never wake up.

The timer is implemented using SIGARLM and alarm(). The intent is to
simply wake up the system call and detect a flag being set by the signal
handler. This relies on using a signal handler that is not restarting
system calls.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
src/ns-support.c
+ if (sigaction(SIGALRM, &act, NULL) < 0) {
+ die("cannot uninstall signal handler for SIGALRM");
+ }
+ if (sanity_timeout_expired) {
@zyga

zyga Sep 14, 2016

Collaborator

I guess this could be done before we reset the alarm really

zyga added some commits Sep 14, 2016

Mark the sanity timeout functions as used
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Tweak messages
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Move flag check to the top of disable call
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
src/ns-support.c
+ // system call we may be sleeping on to get interrupted.
+ if (sigaction(SIGALRM, &act, NULL) < 0) {
+ die("cannot install signal handler for SIGALRM");
+ }
@jdstrand

jdstrand Sep 14, 2016

Contributor

This is more standard (eg, http://www.gnu.org/software/libc/manual/html_node/Sigaction-Function-Example.html):

struct sigaction act;
act.sa_handler = sc_SIGALRM_handler;
if (sigemptyset(&act.sa_mask) < 0) {
    die(...);
}
act.sa_flags = 0;
if (sigaction(SIGINT, &act, NULL) < 0) {
    die(...);
}

If you use this, you can drop the comment since it is clear the flag won't be set.

@zyga

zyga Sep 14, 2016

Collaborator

Done, slightly differently, have a look please.

src/ns-support.c
+ struct sigaction act = { };
+ if (sigaction(SIGALRM, &act, NULL) < 0) {
+ die("cannot uninstall signal handler for SIGALRM");
+ }
@jdstrand

jdstrand Sep 14, 2016

Contributor

Similarly, I think you would want to use this instead:

struct sigaction act;
if (sigemptyset(&act.sa_mask) < 0) {
    die(...);
}
if (sigaction(SIGINT, &act, NULL) < 0) {
    die(...);
}

However, rather than setting this to the empty set, perhaps it would be better to save off the old action in sc_enable_sanity_timeout() and then reinstating it here. The GNU libc URL above shows how to do this; I suspect that may be overkill.

@zyga

zyga Sep 14, 2016

Collaborator

I assume you meant SIGARLM? I also tweaked this to restore SIG_DFL. I don't think we need to restore anything else as we're not using any other signals at this time (YAGNI)

zyga added some commits Sep 14, 2016

Expand documentation for sc_enable_sanity_timeout
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Use sigemptyset explicitly
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Set act.sa_flags = 0 explicitly
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Set sa_handler to SIG_DFL explicitly
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
src/ns-support.c
+ sanity_timeout_expired = 0;
+ struct sigaction act = {.sa_handler = sc_SIGALRM_handler };
+ if (sigemptyset(&act.sa_mask) < 0)
+ die("cannot initialize POSIX signal set");
@jdstrand

jdstrand Sep 14, 2016

Contributor

Nitpick: you aren't using '{}' for a one line body here but you do a few lines below. Does this pass indent?

@zyga

zyga Sep 14, 2016

Collaborator

It does, I'll correct this to { }

src/ns-support.c
+ alarm(0);
+ struct sigaction act = {.sa_handler = SIG_DFL };
+ if (sigemptyset(&act.sa_mask) < 0)
+ die("cannot initialize POSIX signal set");
@jdstrand

jdstrand Sep 14, 2016

Contributor

Same comment here re '{}'

Contributor

jdstrand commented Sep 14, 2016

+1 provided my style comments are addressed.

Always put curly braces after if statement
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@zyga zyga merged commit 8d43d64 into master Sep 14, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@zyga zyga deleted the sanity-timeout branch Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment