Found by Mischa Peters by running some experimental code of mine.

When chaining two proc-based filters together the second proc will not
get its parameter correctly, since lka_filter_process_response won't get
a parameter from the proceed keyword, resulting in a "...|(null)"

The solution seems to be to save the parameter in the filter_session
struct.

While here remove some useless if(NULL) checks and fix two memory leaks
(fs->helo and fs->mail_from).

Mischa has been running with this diff for several days now without
issue.

OK?

martijn@

Index: lka_filter.c
===================================================================
RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v
retrieving revision 1.36
diff -u -p -r1.36 lka_filter.c
--- lka_filter.c        2 May 2019 11:39:45 -0000       1.36
+++ lka_filter.c        30 Jun 2019 14:50:58 -0000
@@ -41,7 +41,7 @@ struct filter;
 struct filter_session;
 static void    filter_protocol_internal(struct filter_session *, uint64_t *, 
uint64_t, enum filter_phase, const char *);
 static void    filter_protocol(uint64_t, enum filter_phase, const char *);
-static void    filter_protocol_next(uint64_t, uint64_t, enum filter_phase, 
const char *);
+static void    filter_protocol_next(uint64_t, uint64_t, enum filter_phase);
 static void    filter_protocol_query(struct filter *, uint64_t, uint64_t, 
const char *, const char *);
 
 static void    filter_data_internal(struct filter_session *, uint64_t, 
uint64_t, const char *);
@@ -68,6 +68,8 @@ struct filter_session {
        uint64_t        id;
        struct io       *io;
 
+       char *lastparam;
+
        char *filter_name;
        struct sockaddr_storage ss_src;
        struct sockaddr_storage ss_dest;
@@ -337,6 +339,9 @@ lka_filter_end(uint64_t reqid)
 
        fs = tree_xpop(&sessions, reqid);
        free(fs->rdns);
+       free(fs->helo);
+       free(fs->mail_from);
+       free(fs->lastparam);
        free(fs);
        log_trace(TRACE_FILTERS, "%016"PRIx64" filters session-end", reqid);
 }
@@ -504,7 +509,7 @@ lka_filter_process_response(const char *
                return 1;
        }
 
-       filter_protocol_next(token, reqid, 0, parameter);
+       filter_protocol_next(token, reqid, 0);
        return 1;
 }
 
@@ -658,14 +663,11 @@ filter_protocol(uint64_t reqid, enum fil
        switch (phase) {
        case FILTER_HELO:
        case FILTER_EHLO:
-               if (fs->helo)
-                       free(fs->helo);
+               free(fs->helo);
                fs->helo = xstrdup(param);
                break;
        case FILTER_MAIL_FROM:
-               if (fs->mail_from)
-                       free(fs->mail_from);
-
+               free(fs->mail_from);
                fs->mail_from = xstrdup(param + 1);
                *strchr(fs->mail_from, '>') = '\0';
                param = fs->mail_from;
@@ -683,13 +685,17 @@ filter_protocol(uint64_t reqid, enum fil
        default:
                break;
        }
+
+       free(fs->lastparam);
+       fs->lastparam = xstrdup(param);
+
        filter_protocol_internal(fs, &token, reqid, phase, param);
        if (nparam)
                free(nparam);
 }
 
 static void
-filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase phase, 
const char *param)
+filter_protocol_next(uint64_t token, uint64_t reqid, enum filter_phase phase)
 {
        struct filter_session  *fs;
 
@@ -697,7 +703,7 @@ filter_protocol_next(uint64_t token, uin
        if ((fs = tree_get(&sessions, reqid)) == NULL)
                return;
 
-       filter_protocol_internal(fs, &token, reqid, phase, param);
+       filter_protocol_internal(fs, &token, reqid, phase, fs->lastparam);
 }
 
 static void

Reply via email to