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);
