[squid-dev] [PATCH] OSX transparent-proxy using pfctl

[Alex Rousskov]

On 09/29/2016 03:48 PM, Shively, Gregory wrote:
>>> I wasn't sure if I should handle it or let it flow up, since if it was
>>> in an overflow state I would doubt I could handle this packet, but
>>> maybe the next connection would be successful.

>> I recommend temporary adding an exception with Must(false) inside the
>> parsing code and testing what happens. This approach may give you enough
>> information to decide whether your code or the caller should be responsible
>> for handling parsing exceptions.



> If I understand correctly, the Must(false) goes all the way up the
> stack to main and exits squid. If instead it falls thru and returns
> false - the connection will fail and the next connection another
> attempt at parsing.

Must(false) just throws an exception. Where that exception will be
caught before main() when thrown from your code, I do not know (perhaps
you already do). Must(false) is just a trick to check whether all
exceptions in your code will be caught/handled the way you want. The
alternative is to trigger real exceptions, but that is often a lot harder.

In general, we should be using as few try/catch as possible to
accomplish the desirable outcome. Thus, always adding try/catch "just in
case" is the wrong approach (I am _not_ saying you are doing that!).
Until Squid exception handling is improved, testing is often the best
way to find the right place.

If you decide that a try/catch is necessary, place it as high as
possible and please do not forget to print the caught exception. There
are examples in the code.


Please note that you do not have to catch exceptions even if they kill
Squid. The current lack of try/catchers around critical contexts is not
your problem. Add try/catch only if you think catching close to your new
code is necessary for your code to work well under more-or-less "normal"
conditions. I doubt it is currently necessary because you are not using
exceptions for benign errors.


> +    if (n == -1) {
> +        int xerrno = errno;
> +        debugs(89, DBG_IMPORTANT, HERE << "Reading from PFCTL failed: " << xstrerr(xerrno));
> +        return false;
> +    }
> +
> +    close(pipefd[0]);

Now leaking pipefd[0] on errors. Someday, Squid will have a auto-close
descriptor wrapper...


> +        SBuf host;
> +        int64_t port;
> +
> +        if (tk.token(host, bracket) || tk.token(host, colon)) {

Nice rewrite! You can move the port declaration lower, and you should.
And no empty lines between the declaration and the single-statement code
that uses that declaration exclusively:

    SBuf host;
    if (...) {
       int64_t port = 0;
       if (...) ...
    }


> +    while (!tk.atEnd()) {
> +        if (tk.token(host, bracket) || tk.token(host, colon)) {
...
> +        }
> +
> +        tk.skip('\n');
> +    }

An infinite loop if the actual input contains, say, a single space and
nothing else? Or a token without brackets and colons? That is unlikely,
of course, but perhaps not completely impossible. I am not dictating
code, but to skip an arbitrary bad line you might need something like
this at the end of an iteration:

  // the combination covers all characters so we will make progress
  (void)tk.skipAll(anythingButLf); // silently skip any garbage
  (void)tk.skipAll(lf))
  // either at the beginning of the next line or eof


BTW, I believe the previous version of your patch had a similar infinite
loop problem, but now it became obvious, thanks to your nice refactoring.


Thank you,

Alex.