On 8/26/19 11:42 AM, Sebastien Marie wrote: > On Sun, Aug 25, 2019 at 07:01:01AM +0200, Martijn van Duren wrote: >> Right now all we get is "Misbehaving filter", which doesn't tell the >> developer a lot. >> >> Diff below does the following: >> - Make the register_hooks actually fail on misbehaviour. >> - Change some *ep = 0 to a more semantically correct ep[0] = '\0' >> - Hoist some checks from lka_filter_process_response to processor_io >> - Make lka_filter_process_response void (it fatals now) >> - strtoull returns ULLONG_MAX on error, not ULONG_MAX >> - change str{,n}casecmp to str{,n}cmp for consistency. >> - change an fugly "nextline:" "goto nextline" loop to an actual while. >> - restructure lka_filter_process_response to be shorter. >> and most importantly >> - Add a descriptive fatal() minefield. >> >> -12 LoC. >> >> Tested with filter-dnsbl with both a successful and rejected connection. >> >> OK? > > just one remark. > >> Index: lka_filter.c >> =================================================================== >> RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v >> retrieving revision 1.41 >> diff -u -p -r1.41 lka_filter.c >> --- lka_filter.c 18 Aug 2019 16:52:02 -0000 1.41 >> +++ lka_filter.c 25 Aug 2019 04:47:47 -0000 >> @@ -429,88 +429,68 @@ lka_filter_process_response(const char * >> struct filter_session *fs; >> >> (void)strlcpy(buffer, line, sizeof buffer); >> - if ((ep = strchr(buffer, '|')) == NULL) >> - return 0; >> - *ep = 0; >> + ep = strchr(buffer, '|'); >> + ep[0] = '\0'; > > is it possible to buffer to not have '|' ? if yes, you could deference NULL. > > You're absolutely right. It's not a risk, since both would crash smtpd, but doing the check would result in a clearer error message, which is the purpose of this endeavour.
Index: lka_filter.c =================================================================== RCS file: /cvs/src/usr.sbin/smtpd/lka_filter.c,v retrieving revision 1.41 diff -u -p -r1.41 lka_filter.c --- lka_filter.c 18 Aug 2019 16:52:02 -0000 1.41 +++ lka_filter.c 26 Aug 2019 10:03:43 -0000 @@ -61,7 +61,7 @@ static void filter_result_reject(uint64_ static void filter_result_disconnect(uint64_t, const char *); static void filter_session_io(struct io *, int, void *); -int lka_filter_process_response(const char *, const char *); +void lka_filter_process_response(const char *, const char *); struct filter_session { @@ -218,13 +218,13 @@ lka_filter_register_hook(const char *nam hook += 8; } else - return; + fatalx("Invalid message direction: %s", hook); for (i = 0; i < nitems(filter_execs); i++) if (strcmp(hook, filter_execs[i].phase_name) == 0) break; if (i == nitems(filter_execs)) - return; + fatalx("Unrecognized report name: %s", hook); iter = NULL; while (dict_iter(&filters, &iter, &filter_name, (void **)&filter)) @@ -414,7 +414,7 @@ filter_session_io(struct io *io, int evt } } -int +void lka_filter_process_response(const char *name, const char *line) { uint64_t reqid; @@ -430,87 +430,68 @@ lka_filter_process_response(const char * (void)strlcpy(buffer, line, sizeof buffer); if ((ep = strchr(buffer, '|')) == NULL) - return 0; - *ep = 0; + fatalx("Missing token: %s", line); + ep[0] = '\0'; kind = buffer; - if (strcmp(kind, "register") == 0) - return 1; - - if (strcmp(kind, "filter-result") != 0 && - strcmp(kind, "filter-dataline") != 0) - return 0; qid = ep+1; if ((ep = strchr(qid, '|')) == NULL) - return 0; - *ep = 0; + fatalx("Missing reqid: %s", line); + ep[0] = '\0'; token = strtoull(qid, &ep, 16); if (qid[0] == '\0' || *ep != '\0') - return 0; - if (errno == ERANGE && token == ULONG_MAX) - return 0; + fatalx("Invalid token: %s", line); + if (errno == ERANGE && token == ULLONG_MAX) + fatal("Invalid token: %s", line); qid = ep+1; if ((ep = strchr(qid, '|')) == NULL) - return 0; - *ep = 0; + fatal("Missing directive: %s", line); + ep[0] = '\0'; reqid = strtoull(qid, &ep, 16); if (qid[0] == '\0' || *ep != '\0') - return 0; - if (errno == ERANGE && reqid == ULONG_MAX) - return 0; + fatalx("Invalid reqid: %s", line); + if (errno == ERANGE && reqid == ULLONG_MAX) + fatal("Invalid reqid: %s", line); response = ep+1; fs = tree_xget(&sessions, reqid); if (strcmp(kind, "filter-dataline") == 0) { if (fs->phase != FILTER_DATA_LINE) - fatalx("misbehaving filter"); + fatalx("filter-dataline out of dataline phase"); filter_data_next(token, reqid, response); - return 1; + return; } if (fs->phase == FILTER_DATA_LINE) - fatalx("misbehaving filter"); + fatalx("filter-result in dataline phase"); if ((ep = strchr(response, '|'))) { parameter = ep + 1; - *ep = 0; + ep[0] = '\0'; } - if (strcmp(response, "proceed") != 0 && - strcmp(response, "reject") != 0 && - strcmp(response, "disconnect") != 0 && - strcmp(response, "rewrite") != 0) - return 0; - - if (strcmp(response, "proceed") == 0 && - parameter) - return 0; - - if (strcmp(response, "proceed") != 0 && - parameter == NULL) - return 0; - - if (strcmp(response, "rewrite") == 0) { - filter_result_rewrite(reqid, parameter); - return 1; - } - - if (strcmp(response, "reject") == 0) { - filter_result_reject(reqid, parameter); - return 1; - } - - if (strcmp(response, "disconnect") == 0) { - filter_result_disconnect(reqid, parameter); - return 1; + if (strcmp(response, "proceed") == 0) { + if (parameter != NULL) + fatalx("Unexpected parameter after proceed: %s", line); + filter_protocol_next(token, reqid, 0); + return; + } else { + if (parameter == NULL) + fatalx("Missing parameter: %s", line); + + if (strcmp(response, "rewrite") == 0) + filter_result_rewrite(reqid, parameter); + else if (strcmp(response, "reject") == 0) + filter_result_reject(reqid, parameter); + else if (strcmp(response, "disconnect") == 0) + filter_result_disconnect(reqid, parameter); + else + fatalx("Invalid directive: %s", line); } - - filter_protocol_next(token, reqid, 0); - return 1; } void Index: lka_proc.c =================================================================== RCS file: /cvs/src/usr.sbin/smtpd/lka_proc.c,v retrieving revision 1.8 diff -u -p -r1.8 lka_proc.c --- lka_proc.c 10 Aug 2019 14:50:58 -0000 1.8 +++ lka_proc.c 26 Aug 2019 10:03:43 -0000 @@ -47,7 +47,7 @@ struct processor_instance { static void processor_io(struct io *, int, void *); static void processor_errfd(struct io *, int, void *); -int lka_filter_process_response(const char *, const char *); +void lka_filter_process_response(const char *, const char *); int lka_proc_ready(void) @@ -123,43 +123,50 @@ processor_register(const char *name, con processor = dict_xget(&processors, name); - if (strcasecmp(line, "register|ready") == 0) { + if (strcmp(line, "register|ready") == 0) { processor->ready = 1; return; } - if (strncasecmp(line, "register|report|", 16) == 0) { + if (strncmp(line, "register|report|", 16) == 0) { lka_report_register_hook(name, line+16); return; } - if (strncasecmp(line, "register|filter|", 16) == 0) { + if (strncmp(line, "register|filter|", 16) == 0) { lka_filter_register_hook(name, line+16); return; } + + fatalx("Invalid register line received: %s", line); } static void processor_io(struct io *io, int evt, void *arg) { + struct processor_instance *processor; const char *name = arg; char *line = NULL; ssize_t len; switch (evt) { case IO_DATAIN: - nextline: - line = io_getline(io, &len); - /* No complete line received */ - if (line == NULL) - return; - - if (strncasecmp("register|", line, 9) == 0) - processor_register(name, line); - else if (! lka_filter_process_response(name, line)) - fatalx("misbehaving filter"); - - goto nextline; + while ((line = io_getline(io, &len)) != NULL) { + if (strncmp("register|", line, 9) == 0) { + processor_register(name, line); + continue; + } + + processor = dict_xget(&processors, name); + if (!processor->ready) + fatalx("Non-register message before register|" + "ready: %s", line); + else if (strncmp(line, "filter-result|", 14) == 0 || + strncmp(line, "filter-dataline|", 16) || 0) + lka_filter_process_response(name, line); + else + fatalx("Invalid filter message type: %s", line); + } } } Index: lka_report.c =================================================================== RCS file: /cvs/src/usr.sbin/smtpd/lka_report.c,v retrieving revision 1.24 diff -u -p -r1.24 lka_report.c --- lka_report.c 18 Aug 2019 16:52:02 -0000 1.24 +++ lka_report.c 26 Aug 2019 10:03:43 -0000 @@ -102,16 +102,16 @@ lka_report_register_hook(const char *nam void *iter; size_t i; - if (strncasecmp(hook, "smtp-in|", 8) == 0) { + if (strncmp(hook, "smtp-in|", 8) == 0) { subsystem = &smtp_in; hook += 8; } - else if (strncasecmp(hook, "smtp-out|", 9) == 0) { + else if (strncmp(hook, "smtp-out|", 9) == 0) { subsystem = &smtp_out; hook += 9; } else - return; + fatalx("Invalid message direction: %s", hook); if (strcmp(hook, "*") == 0) { iter = NULL; @@ -127,7 +127,7 @@ lka_report_register_hook(const char *nam if (strcmp(hook, smtp_events[i].event) == 0) break; if (i == nitems(smtp_events)) - return; + fatalx("Unrecognized report name: %s", hook); tailq = dict_get(subsystem, hook); rp = xcalloc(1, sizeof *rp);