Victor wrote:
> It looks like the ps_early_dnsbl_event() callback is called here with
> a client context that has been deallocated (client disconnected, ...
> before all the DNS blacklist lookups completed). The callback should
> be cancelled when the client state is deleted.
>
> The "dnsbl_score_cache" table contains per-client address call-back
> lists for each connection that wants the score results, and this callback
> list does not appear to be pruned when a client disconnects...
The following patch for postfix-2.8-20100923 eliminates two race
conditions. Both are triggered when a client makes N > 1 simultaneous
connections, and then disconnects M < N connections before postscreen
has delivered the DNSBL score to the "pseudo" threads for those M
connections.
This problem was introduced with postfix-2.8-20100914.
The patch looks large, but that is because it adds one additional
argument to a function that is called from several places.
I have not tried to apply the patch to postfix versions between
postfix-2.8-20100914 and postfix-2.8-20100923. Instead, I'll
upload an updated snapshot later today.
Wietse
diff -cr /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen.h
src/postscreen/postscreen.h
*** /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen.h Thu Sep 23
11:22:30 2010
--- src/postscreen/postscreen.h Wed Oct 6 15:06:13 2010
***************
*** 49,54 ****
--- 49,55 ----
time_t pregr_stamp; /* pregreet expiration time */
time_t dnsbl_stamp; /* dnsbl expiration time */
VSTRING *dnsbl_reply; /* dnsbl reject text */
+ int dnsbl_index; /* dnsbl request index */
/* Built-in SMTP protocol engine. */
time_t pipel_stamp; /* pipelining expiration time */
time_t nsmtp_stamp; /* non-smtp command expiration time */
***************
*** 378,385 ****
* postscreen_dnsbl.c
*/
extern void ps_dnsbl_init(void);
! extern int ps_dnsbl_retrieve(const char *, const char **);
! extern void ps_dnsbl_request(const char *, void (*) (int, char *), char *);
/*
* postscreen_tests.c
--- 379,386 ----
* postscreen_dnsbl.c
*/
extern void ps_dnsbl_init(void);
! extern int ps_dnsbl_retrieve(const char *, const char **, int);
! extern int ps_dnsbl_request(const char *, void (*) (int, char *), char *);
/*
* postscreen_tests.c
diff -cr /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen_dnsbl.c
src/postscreen/postscreen_dnsbl.c
*** /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen_dnsbl.c Thu Sep
16 08:01:40 2010
--- src/postscreen/postscreen_dnsbl.c Wed Oct 6 16:11:37 2010
***************
*** 8,21 ****
/*
/* void ps_dnsbl_init(void)
/*
! /* void ps_dnsbl_request(client_addr, callback, context)
/* char *client_addr;
/* void (*callback)(int, char *);
/* char *context;
/*
! /* int ps_dnsbl_retrieve(client_addr, dnsbl_name)
/* char *client_addr;
/* const char **dnsbl_name;
/* DESCRIPTION
/* This module implements preliminary support for DNSBL lookups.
/* Multiple requests for the same information are handled with
--- 8,22 ----
/*
/* void ps_dnsbl_init(void)
/*
! /* int ps_dnsbl_request(client_addr, callback, context)
/* char *client_addr;
/* void (*callback)(int, char *);
/* char *context;
/*
! /* int ps_dnsbl_retrieve(client_addr, dnsbl_name, dnsbl_index)
/* char *client_addr;
/* const char **dnsbl_name;
+ /* int dnsbl_index;
/* DESCRIPTION
/* This module implements preliminary support for DNSBL lookups.
/* Multiple requests for the same information are handled with
***************
*** 33,38 ****
--- 34,41 ----
/* on to the callback function. The callback should ignore its
/* first argument (it exists for compatibility with Postfix
/* generic event infrastructure).
+ /* The result value is the index for the ps_dnsbl_retrieve()
+ /* call.
/*
/* ps_dnsbl_retrieve() retrieves the result score requested with
/* ps_dnsbl_request() and decrements the reference count. It
***************
*** 141,146 ****
--- 144,162 ----
(sp)->index = 0; \
} while (0)
+ #define PS_CALL_BACK_INDEX_OF_LAST(sp) ((sp)->index - 1)
+
+ #define PS_CALL_BACK_CANCEL(sp, idx) do { \
+ PS_CALL_BACK_ENTRY *_cb_; \
+ if ((idx) < 0 || (idx) >= (sp)->index) \
+ msg_panic("%s: index %d must be >= 0 and < %d", \
+ myname, (idx), (sp)->index); \
+ _cb_ = (sp)->table + (idx); \
+ event_cancel_timer(_cb_->callback, _cb_->context); \
+ _cb_->callback = 0; \
+ _cb_->context = 0; \
+ } while (0)
+
#define PS_CALL_BACK_EXTEND(hp, sp) do { \
if ((sp)->index >= (sp)->limit) { \
int _count_ = ((sp)->limit ? (sp)->limit * 2 : 5); \
***************
*** 160,166 ****
#define PS_CALL_BACK_NOTIFY(sp, ev) do { \
PS_CALL_BACK_ENTRY *_cb_; \
for (_cb_ = (sp)->table; _cb_ < (sp)->table + (sp)->index; _cb_++) \
! _cb_->callback((ev), _cb_->context); \
} while (0)
#define PS_NULL_EVENT (0)
--- 176,183 ----
#define PS_CALL_BACK_NOTIFY(sp, ev) do { \
PS_CALL_BACK_ENTRY *_cb_; \
for (_cb_ = (sp)->table; _cb_ < (sp)->table + (sp)->index; _cb_++) \
! if (_cb_->callback != 0) \
! _cb_->callback((ev), _cb_->context); \
} while (0)
#define PS_NULL_EVENT (0)
***************
*** 264,270 ****
/* ps_dnsbl_retrieve - retrieve blocklist score, decrement reference count */
! int ps_dnsbl_retrieve(const char *client_addr, const char **dnsbl_name)
{
const char *myname = "ps_dnsbl_retrieve";
PS_DNSBL_SCORE *score;
--- 281,288 ----
/* ps_dnsbl_retrieve - retrieve blocklist score, decrement reference count */
! int ps_dnsbl_retrieve(const char *client_addr, const char **dnsbl_name,
! int dnsbl_index)
{
const char *myname = "ps_dnsbl_retrieve";
PS_DNSBL_SCORE *score;
***************
*** 278,283 ****
--- 296,306 ----
msg_panic("%s: no blocklist score for %s", myname, client_addr);
/*
+ * Disable callbacks.
+ */
+ PS_CALL_BACK_CANCEL(score, dnsbl_index);
+
+ /*
* Reads are destructive.
*/
result_score = score->total;
***************
*** 376,382 ****
/* ps_dnsbl_request - send dnsbl query, increment reference count */
! void ps_dnsbl_request(const char *client_addr,
void (*callback) (int, char *),
char *context)
{
--- 399,405 ----
/* ps_dnsbl_request - send dnsbl query, increment reference count */
! int ps_dnsbl_request(const char *client_addr,
void (*callback) (int, char *),
char *context)
{
***************
*** 420,426 ****
score->pending_lookups);
if (score->pending_lookups == 0)
event_request_timer(callback, context, EVENT_NULL_DELAY);
! return;
}
if (msg_verbose > 1)
msg_info("%s: create blocklist score for %s", myname, client_addr);
--- 443,449 ----
score->pending_lookups);
if (score->pending_lookups == 0)
event_request_timer(callback, context, EVENT_NULL_DELAY);
! return (PS_CALL_BACK_INDEX_OF_LAST(score));
}
if (msg_verbose > 1)
msg_info("%s: create blocklist score for %s", myname, client_addr);
***************
*** 458,463 ****
--- 481,487 ----
(char *) stream, DNSBLOG_TIMEOUT);
score->pending_lookups += 1;
}
+ return (PS_CALL_BACK_INDEX_OF_LAST(score));
}
/* ps_dnsbl_init - initialize */
diff -cr /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen_early.c
src/postscreen/postscreen_early.c
*** /var/tmp/postfix-2.8-20100923/src/postscreen/postscreen_early.c Tue Sep
21 20:02:24 2010
--- src/postscreen/postscreen_early.c Wed Oct 6 16:15:32 2010
***************
*** 110,116 ****
if (state->flags & PS_STATE_FLAG_DNSBL_TODO) {
dnsbl_score =
! ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name);
if (dnsbl_score < var_ps_dnsbl_thresh) {
state->dnsbl_stamp = event_time() + var_ps_dnsbl_ttl;
PS_PASS_SESSION_STATE(state, "dnsbl test",
--- 110,117 ----
if (state->flags & PS_STATE_FLAG_DNSBL_TODO) {
dnsbl_score =
! ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name,
! state->dnsbl_index);
if (dnsbl_score < var_ps_dnsbl_thresh) {
state->dnsbl_stamp = event_time() + var_ps_dnsbl_ttl;
PS_PASS_SESSION_STATE(state, "dnsbl test",
***************
*** 166,172 ****
read_buf, sizeof(read_buf) - 1, MSG_PEEK)) <= 0) {
/* Avoid memory leak. */
if (state->flags & PS_STATE_FLAG_DNSBL_TODO)
! (void) ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name);
/* XXX Wait for DNS replies to come in. */
ps_hangup_event(state);
return;
--- 167,174 ----
read_buf, sizeof(read_buf) - 1, MSG_PEEK)) <= 0) {
/* Avoid memory leak. */
if (state->flags & PS_STATE_FLAG_DNSBL_TODO)
! (void) ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name,
! state->dnsbl_index);
/* XXX Wait for DNS replies to come in. */
ps_hangup_event(state);
return;
***************
*** 180,186 ****
case PS_ACT_DROP:
/* Avoid memory leak. */
if (state->flags & PS_STATE_FLAG_DNSBL_TODO)
! (void) ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name);
PS_DROP_SESSION_STATE(state, "521 5.5.1 Protocol error\r\n");
return;
case PS_ACT_ENFORCE:
--- 182,189 ----
case PS_ACT_DROP:
/* Avoid memory leak. */
if (state->flags & PS_STATE_FLAG_DNSBL_TODO)
! (void) ps_dnsbl_retrieve(state->smtp_client_addr, &dnsbl_name,
! state->dnsbl_index);
PS_DROP_SESSION_STATE(state, "521 5.5.1 Protocol error\r\n");
return;
case PS_ACT_ENFORCE:
***************
*** 266,273 ****
* Run a DNS blocklist query.
*/
if ((state->flags & PS_STATE_FLAG_DNSBL_TODO) != 0)
! ps_dnsbl_request(state->smtp_client_addr, ps_early_dnsbl_event,
! (char *) state);
/*
* Wait for the client to respond or for DNS lookup to complete.
--- 269,279 ----
* Run a DNS blocklist query.
*/
if ((state->flags & PS_STATE_FLAG_DNSBL_TODO) != 0)
! state->dnsbl_index =
! ps_dnsbl_request(state->smtp_client_addr, ps_early_dnsbl_event,
! (char *) state);
! else
! state->dnsbl_index = -1;
/*
* Wait for the client to respond or for DNS lookup to complete.
