On Wed, 21 Jun 2023 19:11:09 +0200, Omar Polo wrote:
> On 2023/06/20 14:38:37 -0600, Todd C. Miller <[email protected]> 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
