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. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev