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

[Shively, Gregory]

> -----Original Message-----
> From: Alex Rousskov [mailto:rousskov at measurement-factory.com]
> Sent: Wednesday, September 28, 2016 6:05 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/28/2016 12:18 PM, Shively, Gregory wrote:
> > 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?
> 
> If nobody recommends a better place, then place the warning in your code.
> "bzr grep WARNING src" for examples.
> 
> 

Will put it into the code for now.

> >> 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.
> 
> I did not send a merge document. You should run the test-builds.sh. I do not
> know whether running test-builds.sh will expose any bugs you are
> responsible for. If running test-builds.sh does not expose any bugs you are
> responsible for [because of all the other bugs that you are not responsible
> for], then simply say so.
> 
> 

Sorry - I think it was Amos instead. Sometimes these mailing lists make me think like I'm talking to one person :-). But that is the problem that I'm having - I can't even run the test suite on a OSX machine prior to making any changed. Running the test suite ends in the btlayer-00-default with an error that stops the build during dist-clean due to:

ERROR: files left in build directory after distclean:
./src/cf_gen.dSYM/Contents/Info.plist
./src/cf_gen.dSYM/Contents/Resources/DWARF/cf_gen

Based on the names, my guess is that the Xcode compiler is generating some artifacts that dist-clean isn't expecting. I did find that I can run with the "--keep-going" flag, but that ends with a the equivalent error in btlayer-05-no-deps-esi. Do I run it this way and just grep the logs for FAIL or something?


> >>> +    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.
> 
> > Does just using the appendf() method sound alright?
> 
> Sounds bad to me. Just use SBuf::append() to concatenate strings and
> characters instead of turning off many C++ protections by using printf() or
> equivalent.
> 
> 

Understood - I will use SBuf::append, for some reason I was thinking that I had the port as a numeric - does seem better to use append.

> > And am I
> > correct that the method with throw an exception that should just rollup the
> stack?
> 
> Yes, SBuf methods should throw on overflows and similar non-logic errors.
> Whether your code can handle those exceptions correctly, I have not
> checked.
> 
> 

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.

> >>> +    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.
> 
> It is not just a pointer -- there would be no reason to add all those
> f*() functions if it were! FILE is a libc object with buffers. It also includes a
> pointer to the kernel structures, of course.
> 
> 
> > I'll go ahead and change it to a fclose()
> 
> Yes, that is the right solution if you insist on using FILE.
> 
> 
> >> 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.
> 
> My recommendation was given with the above assumption in mind.
> 
> 
> > 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.
> 
> Yes, but it is not a problem if you parse correctly. Currently, you are
> combining partial parsing with partial reading. Splitting them often avoids
> nasty bugs that result from such combination.
> 
> 
> > I could use Tokenizer or it looks like Just SBuf
> 
> You should use Tokenizer instead of manually parsing what looks like trivial
> syntax. Parsing problems in the first patch version imply that it is not as trivial
> as it may seem (or, to be more precise, parsing trivial syntax by hand is still
> error-prone!).
> 
> 
> > if I was to use just read(), I'd have to read a character at a time
> > from the fd
> 
> Why not read, say, 4096 characters at a time?
> 
> 
> > .... or if you want me to throw the input stream into a SBuf or
> > Tokenizer instead of just using some C calls.
> 
> I do, but I cannot insist on that. You can stop polishing the patch as soon as it
> has no known bugs. Others will decide whether to accept it.
> 
> 

I appreciate the suggestions, both from a standpoint of not having been writing C/C++ code for a long time and I'd rather have the code be more "consistent" with the rest of the application. That's why using some of the classes that is already in the app that I had no idea was even there I think is a good idea. The reading a char at a time - I was thinking about parsing as reading. Anyway, I had already started it prior to reading the "stop polishing".

> Please note that you have not shared updated changes AFAICT. The squid-
> 3.5.20-osx-devpf-transparent.patch you attached looked old to me.
> 
> 

You are right - such a pain in the environment that I'm developing in. Mac off in a segmented network, which doesn’t have access to where I'm emailing from. Let alone having to change proxy settings. So have to dump the patch on a jump host, which I forgot to update with the new and copied the old file down to my email host. This one should be updated.

> Thank you,
> 
> Alex.

Thank you for the time.
Greg

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