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

[Alex Rousskov]

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.


>> 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.


>>> +    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.


> 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.


>>> +    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.


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.


Thank you,

Alex.