25 août 2019 07:01 "Martijn van Duren" <openbsd+t...@list.imperialat.at> a écrit: > Right now all we get is "Misbehaving filter", which doesn't tell the > developer a lot. >
Indeed > 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? > > martijn@ > Reads fine but I'll give it a better read today and run with it on my machines for a day see if I observe any issues with filter-rspamd and filter-senderscore as well as some builtin filters. > 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 > @@ -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; > @@ -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'; > > 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 25 Aug 2019 04:47:47 -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 25 Aug 2019 04:47:47 -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; > @@ -126,8 +126,9 @@ lka_report_register_hook(const char *nam > for (i = 0; i < nitems(smtp_events); i++) > if (strcmp(hook, smtp_events[i].event) == 0) > break; > - if (i == nitems(smtp_events)) > - return; > + if (i == nitems(smtp_events)) { > + fatalx("Unrecognized report name: %s", hook); > + } > > tailq = dict_get(subsystem, hook); > rp = xcalloc(1, sizeof *rp);