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

[Shively, Gregory]

> -----Original Message-----
> From: Alex Rousskov [mailto:rousskov at measurement-factory.com]
> Sent: Monday, September 26, 2016 4:07 PM
> To: squid-dev at lists.squid-cache.org
> Cc: Shively, Gregory <gregory_shively at fanniemae.com>
> Subject: [EXTERNAL] Re: [squid-dev] [PATCH] OSX transparent-proxy using
> pfctl
> 
> On 09/26/2016 12:59 PM, Shively, Gregory wrote:
> 
> > The patch calls /sbin/pfctl to get the redirect state information
> 
> For every intercepted connection, this patch forks Squid to start a shell
> (which then starts pfctl and awk) and then blocks Squid on that shell output,
> right? That feels very expensive performance-wise. I wonder whether Squid
> should emit a corresponding one-time warning, to minimize the number of
> folks who are going to assume that this works well under load and then
> complain that their Squid is "slow".
> 
> The awk part can be eliminated by parsing in Squid, but I am not sure that
> optimization is worth it when there is so much overhead in this code besides
> awk.
> 
> 

Totally agree with that. For my use case, I'm using squid for a small number of 
embedded devices that don't have the ability to connect to the available WiFi 
network. The one-time warning sounds like a good idea. Is there a place that
you have to add the one-time message, or should I just add a static variable 
to determine if the warning has been displayed the first time down this code
path?

I had started to parse the results of the pfctl in squid, but came to the same
conclusion and decided to just leave the awk.

> > Let me know if I should continue down the road on getting
> > test-builds.sh running on OSX.
> 
> IMO, you should not be responsible for fixing any old test-builds.sh bugs,
> only for the problems your changes add or cause.
> 
> 

Thanks - I was mistaken and thought from the merge document you sent that
I should have run the test-builds.sh. 

> > +    char *cmd = (char *)malloc(sizeof(char) * cmdLen);
> > +    snprintf(cmd, cmdLen, cmdFormat, daddr, saddr, established);
> 
> Please rewrite using SBuf. AFAICT, what you want can be written without all
> those unsafe C things like malloc() and snprintf() -- you are simply
> concatenating a few strings to form a command.
> 
> 

Makes sense; I've been out of writing C for quite a while and never was much of 
a C++ programmer. Does just using the appendf() method sound alright? And am I
correct that the method with throw an exception that should just rollup the stack?

> > +        execl("/bin/sh", "/bin/sh", "-c", cmd, NULL);
> 
> Please propagate or otherwise handle execl() errors.
> 
> 
> > +    FILE *fp = fdopen(pipefd[0], "r");
> > +    while (!feof(fp)) {
> 
> Please handle fdopen() errors instead of ignoring them and feeding a nil
> pointer to feof().
> 
> 

Also added the check to return of the pipe().

> > +    FILE *fp = fdopen(pipefd[0], "r");
> ...
> > +            close(pipefd[0]);
> > +            return true;
> ...
> > +    close(pipefd[0]);
> > +    return false;
> 
> Leaking "fp"? AFAICT, you are supposed to close "fp" instead of pipefd[0]
> after fdopen() but I am not sure.
> 

> 
> > +                debugs(89, DBG_IMPORTANT, HERE << "PFCTL failed to find
> state");
> > +                return false;
> 
> Leaking both "fp" and pipefd[0]?
> 
> 

If my memory serves, I think the FILE * returned from fdopen() is just a pointer to the 
same datastructure in the kernel that fd would be associated to. So I was under the 
impression that either a fclose(fp) or close(fd) should be equivalent. I wonder if closing
both would also be bad. I found a SO http://stackoverflow.com/questions/13691107/c-proper-way-to-close-files-when-using-both-open-and-fdopen.
I'll go ahead and change it to a fclose() - don't think it matters, but might be more consistent
since I was using buffered reading. But from here, I think, that we should only close
either fp or pipefd[0].

> > +    char rdaddr[MAX_IPSTRLEN + 6];
> 
> AFAICT, the input line may have more characters than that because ":", "\n",
> and "\0" all consume space (I assume 6 is for the ":port").
> 
> Also, this declaration is not needed outside the while loop.
> 
> 

Will add the 3 extra characters. Is the MAX_IPSTRLEN that correct macro to use here?
I wasn't sure of what the max length of a IPv6 string and when I looked it up, I think
MAX_IPSTRLEN was a bit longer.

> > +            char *portPtr = strchr(rdaddr, '\n');
> > +            if (portPtr) *portPtr = '\0';
> 
> Should not we reject truncated lines (without '\n') instead of hoping that the
> port number or the address itself was not truncated?
> 
> 

Makes sense.

> > +        if (errno == EINTR || errno == EAGAIN) {
> > +            continue;
> > +        }
> 
> This code has no effect AFAICT -- the loop will continue with or without that if
> statement. Did you mean to break the loop on all other errors instead?
> 
> 

That was left over code that I forgot to pull out. When I was using popen(), for
some reason, the 2nd (or maybe 3rd) connection was returning an error
on fgets(). I was just trying to figure out what was going on.

> I recommend removing "fp" and reading from pipefd[0] directly instead.
> You can use Tokenizer to safely parse what you read without these error-
> prone C tricks.
> 

The reason that I used fgets() instead of read() on the pipefd[0] is that I'm 
reading either the  length of rdaddr or until I read a newline, which ever 
comes first. If I used read(), if the length of the line was less than rdaddr len
(and there was more than one line), it could read the beginning of the next
Line. At least that is what I remember. I could use Tokenizer or it looks like
Just SBuf after I get the line since the tokenizing is pretty simple. But I think
 if I was to use just read(), I'd have to read a character at a time from the fd
and at that point would it be just better to keep track of when I hit the ':'
in the input stream. I could do that if you'd like.


> > +		CPPFLAGS=\"-D_SQUID_APPLE -Wno-error=deprecated-
> declarations\"
> > +LDFLAGS=-lresolv \
> ...
> > -DISTCHECK_CONFIGURE_FLAGS=""
> > +DISTCHECK_CONFIGURE_FLAGS="CPPFLAGS=\"-D_SQUID_APPLE -Wno-
> error=deprecated-declarations\" LDFLAGS=-lresolv"
> 
> These temporary for-testing changes should not be a part of the patch.
> 
> 

Sorry - had meant to remove those prior to sending. I've made the changes other than
removing the use of fp and using the pipefd[0]. I can still make that change also, but
just wanted your feedback if it should be changed, if I missed something in
Tokenizer, and/or if you want me to throw the input stream into a SBuf or
Tokenizer instead of just using some C calls.




> HTH,
> 
> Alex.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: squid-3.5.20-osx-devpf-transparent.patch
Type: application/octet-stream
Size: 4139 bytes
Desc: squid-3.5.20-osx-devpf-transparent.patch
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160928/565ace82/attachment.obj>