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

[Amos Jeffries]

On 1/10/2016 4:04 a.m., Shively, Gregory wrote:
>> 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.
>> 
>> 
> 
> I think I'll just leave the exception handling for the caller then.
> There really isn't anything that can be done to resolve the exception
> in the callee.
> 
>>> +    if (n == -1) { +        int xerrno = errno; +
>>> debugs(89, DBG_IMPORTANT, HERE << "Reading from PFCTL failed: "

NP: please drop the HERE macro in new code. It is deprecated now and not
needed.

>> << xstrerr(xerrno));
>>> +        return false; +    } + +    close(pipefd[0]);
>> 
>> Now leaking pipefd[0] on errors. Someday, Squid will have a
>> auto-close descriptor wrapper...
>> 
>> 
> 
> Thanks - I felt list I was missing something there :-).
> 
>>> +        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.
>> 
>> 
> 
> You are right - there is an infinite loop. I started to play with the
> tokenizing after I sent the patch and didn't see the infinite loop,
> but I don't think it is doing what I was attempting in the loop
> anyway. How about I get rid of the loop all together - I should be
> only getting one line from pfctl, and if the parsing fails -I should
> probably just be returning false anyway instead of attempting to find
> what I expect later in the buffer. I was trying to be safe from the
> parsing, but I'm starting to think I should just fail out of the
> parsing if the buffer doesn't have what I expect.

Provided one line is always sufficient to get the NAT'ed address or to
detect a problem. Then yes returning false/fail is the right thing to do
there.

The one thing to be very careful of here is that results for different
IP addresses cannot get mixed up. The entire purpose of this lookup is
to guarantee reliability of the information being used. If it fails
people face real costs and painful trouble.

Please make sure that your code debugs() dumps the full pfctl line(s)
received at level DBG_DATA, and (only) on errors the relevant bit at a
higher level like 2 or 3 - the other functions debug output can give


Some other things about the current patch:

* for the address string buffers SQUIDHOSTNAMELEN is guaranteed to be
long enough to store a URL-format IP address with port information.
MAX_IPSTRLEN is only guaranteed to be able to hold one IP by itself (the
'extra' space is for some weird formats that can be found in the more
unusual corners of networking).


> 
> A different question - I'm wondering if I should move where this
> conditional compilation is being placed with regard to the other
> conditional compilation. I had put it in based on what the last
> compilation options I had used when attempting to get this working
> before looking in the code. I didn't know if this would be enabled by
> configure or some other macro that might be defined on OSX. But it
> looks like I have it inside the conditional compilation for
> !USE_NAT_DEVPF. Do you think it should be moved?

It might be best if the conditionals were re-arranged like so:

#if USE_NAT_DEVPF
... the /dev/pf stuff

#elif _SQUID_APPLE_
... this new pfctl stuff

#else /* !USE_NATDEVPF */
... the getsockname stuff ...

#endif

Since they are a inside the --enable-pf-transparent conditional
PF_TRANSPARENT wrapper, that should be all that is needed to enable it
on MacOSX.


> 
> Regarding the test_builds.sh - got it to run with the --keep-going
> and grepped out of the logs - only PASS in the summary section. This
> was with the current patch. I'll run it again, but at least know that
> it is working.

Not something to worry about with this patch. That testing is more for
changes where behaviour with and without the feature being enabled gets
affected and needs to be tested.

This change is pretty isolated and you are not changing any of the
behaviour when its disabled. So you only need to test the build using
this new code works as expected.

The fact that it is failing might be a sign of other MacOS issues that
need attention unrelated to this patching. But may also be just missing
commonly used dependencies on your machine.


Amos