2013-03-17 16:07 keltezéssel, Tom Lane írta:
Boszormenyi Zoltan <z...@cybertec.at> writes:
2013-03-17 04:48 keltezéssel, Tom Lane írta:
[ worries about stray SIGALRM events ]
Your reasoning seems to be correct. However, if we take it to the
extreme, enable_timeout_at/enable_timeout_after/enable_timeouts
should also disable the interrupt handler at the beginning of the
function and enabled at the end with pqsignal(). An evil soul may
send SIGALRM externally to the backend processes at the proper
moment and create a race condition while enable_timeout*() is in
progress and the itimer is about to trigger at the same time (but
doesn't since it was disabled).
Well, a malefactor with the ability to send signals to a backend
process could also send SIGKILL, or any number of other signals that
would mess things up. I'm not too concerned about that scenario.
I *am* concerned about leftover timer events, both because this code
isn't very well tested, and because third-party code loaded into the
backend (think pl/perl for instance) could easily set up timer events
we weren't expecting.
I see.
It suddenly occurs to me though that there's more than one way to skin
this cat. We could easily add another static flag variable called
"sigalrm_allowed" or some such, and have the signal handler test that
and immediately return without doing anything if it's off. Clearing
and setting such a variable would be a lot cheaper than an extra
setitimer call, as well as more robust since it would protect us from
all sources of SIGALRM not just ITIMER_REAL.
Something like the attached patch?
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index b5a3c8f..314a601 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -49,52 +49,29 @@ static bool all_timeouts_initialized = false;
static volatile int num_active_timeouts = 0;
static timeout_params *volatile active_timeouts[MAX_TIMEOUTS];
+/*
+ * Safeguard for the signal handler.
+ */
+static volatile bool alarm_enabled = false;
/*****************************************************************************
* Internal helper functions
*
* For all of these, it is caller's responsibility to protect them from
- * interruption by the signal handler. Generally, call disable_alarm()
- * first to prevent interruption, then update state, and last call
+ * interruption by the signal handler. Generally, set alarm_enabled to false
+ * first to prevent interrupt to do anything, then update state, and last call
* schedule_alarm(), which will re-enable the interrupt if needed.
- *****************************************************************************/
-
-/*
- * Disable alarm interrupts
*
- * multi_insert must be true if the caller intends to activate multiple new
- * timeouts. Otherwise it should be false.
- */
-static void
-disable_alarm(bool multi_insert)
-{
- struct itimerval timeval;
+ * There may be a pending interrupt during enable_timeout_at(),
+ * enable_timeout_after() or enablwe_timeouts(). But since alarm_enabled is
+ * false, it is harmless when it triggers. We will reschedule the interrupt
+ * that just fired. In the worst case, it will be late by one unit of the
+ * the timer resolution in the OS.
+ *
+ *****************************************************************************/
- /*
- * If num_active_timeouts is zero, and multi_insert is false, we don't
- * have to call setitimer. There should not be any pending interrupt, and
- * even if there is, the worst possible case is that the signal handler
- * fires during schedule_alarm. (If it fires at any point before
- * insert_timeout has incremented num_active_timeouts, it will do nothing,
- * since it sees no active timeouts.) In that case we could end up
- * scheduling a useless interrupt ... but when the extra interrupt does
- * happen, the signal handler will do nothing, so it's all good.
- *
- * However, if the caller intends to do anything more after first calling
- * insert_timeout, the above argument breaks down, since the signal
- * handler could interrupt the subsequent operations leading to corrupt
- * state. Out of an abundance of caution, we forcibly disable the timer
- * even though it should be off already, just to be sure. Even though
- * this setitimer call is probably useless, we're still ahead of the game
- * compared to scheduling two or more timeouts independently.
- */
- if (multi_insert || num_active_timeouts > 0)
- {
- MemSet(&timeval, 0, sizeof(struct itimerval));
- if (setitimer(ITIMER_REAL, &timeval, NULL) != 0)
- elog(FATAL, "could not disable SIGALRM timer: %m");
- }
-}
+#define disable_alarm() \
+ do { alarm_enabled = false; } while (0)
/*
* Find the index of a given timeout reason in the active array.
@@ -228,6 +205,14 @@ schedule_alarm(TimestampTz now)
timeval.it_value.tv_sec = secs;
timeval.it_value.tv_usec = usecs;
+ /*
+ * Enable the interrupt handler, just before calling setitimer(),
+ * so in case SIGALRM triggers just after returning from the
+ * syscall, it won't be a no-op. Use the do { ... } while (0)
+ * trick so the compiler doesn't reorder this statement.
+ */
+ do { alarm_enabled = true; } while (0);
+
/* Set the alarm timer */
if (setitimer(ITIMER_REAL, &timeval, NULL) != 0)
elog(FATAL, "could not enable SIGALRM timer: %m");
@@ -248,7 +233,12 @@ schedule_alarm(TimestampTz now)
static void
handle_sig_alarm(SIGNAL_ARGS)
{
- int save_errno = errno;
+ int save_errno;
+
+ if (!alarm_enabled)
+ return;
+
+ save_errno = errno;
/*
* SIGALRM is always cause for waking anything waiting on the process
@@ -376,7 +366,7 @@ enable_timeout_after(TimeoutId id, int delay_ms)
TimestampTz fin_time;
/* Disable timeout interrupts for safety. */
- disable_alarm(false);
+ disable_alarm();
/* Queue the timeout at the appropriate time. */
now = GetCurrentTimestamp();
@@ -424,7 +414,7 @@ enable_timeouts(const EnableTimeoutParams *timeouts, int count)
int i;
/* Disable timeout interrupts for safety. */
- disable_alarm(count > 1);
+ disable_alarm();
/* Queue the timeout(s) at the appropriate times. */
now = GetCurrentTimestamp();
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
