On Wed, 21 Jun 2023 19:11:09 +0200, Omar Polo wrote: > On 2023/06/20 14:38:37 -0600, Todd C. Miller <mill...@openbsd.org> wrote: > > > qid = ep+1; > > > - if ((ep = strchr(qid, '|')) == NULL) > > > - fatalx("Missing reqid: %s", line); > > > - ep[0] = '\0'; > > > - > > > > This is not a new problem but we really should set errno=0 before > > calling strtoull() for the first time. It is possible for errno > > to already be set to ERANGE which causes problems if strtoull() > > returns ULLONG_MAX and there is not an error. It's fine if you > > don't want to fix that in this commit but I do think it should get > > fixed. > > if you don't mind i'll fix it in a separate commit. I've also checked > if there were other places to adjust but this appears to be the only > one instance.
That's perfectly fine. > > It took me a minute to realize that this is OK, but it seems fine. > > > > > if (strcmp(response, "proceed") == 0) { > > > - if (parameter != NULL) > > > - fatalx("Unexpected parameter after proceed: %s", line); > > > filter_protocol_next(token, reqid, 0); > > > return; > > yeah it's subtle, i should have pointed it out, sorry. if "response" > contain a parameter, strcmp() return nonzero, so the parameter check > is not needed. > > The drawback is that the error messages on protocol violation are a > bit worst. Before something like "...|proceed|foobar" would fail with > "unexpected parameter after proceed: <line>", now "Invalid directive: > <line>", but I don't think it's a big deal. I noticed this and I agree that it is not a big deal. If you are writing a filter you should know enough to debug the problem with the amount of detail provided. OK millert@ for the diff as-is if it was not obvious from my previous reply. - todd