On Mon, Aug 26, 2019 at 12:04:06PM +0200, Martijn van Duren wrote:
> 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.
>
ok gilles@
> 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);
>
--
Gilles Chehade @poolpOrg
https://www.poolp.org patreon: https://www.patreon.com/gilles
