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

[Alex Rousskov]

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.


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


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


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


> +    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]?


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


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


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


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.

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


HTH,

Alex.