postfix-users October 2010 archive
Main Archive Page > Month Archives  > postfix-users archives
postfix-users: PATCH: postscreen segfault

PATCH: postscreen segfault

From: Wietse Venema <wietse_at_nospam>
Date: Wed Oct 06 2010 - 20:56:21 GMT
To: Postfix users <postfix-users@postfix.org>

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.