Re: Patches
James Vasile <[email protected]>
| Newsgroups | gmane.comp.web.privoxy.devel |
|---|---|
| Message-ID | <[email protected]> |
On Mon, 16 Jan 2012 21:07:12 +0100, Fabian Keil <[email protected]> wrote: > James Vasile <[email protected]> wrote: > > > Roland and Fabien, > > > > First, thanks for your work with Privoxy. It's excellent! > > Thanks. > > Note that there are other Privoxy developers who may be > interested in your work, so if you don't mind discussing > this in public, please CC [email protected] > when replying. > > > I work with the FreedomBox project. We're integrating privacy, security > > and anonymity infrastructure in a plug server for easy end-user use. > > Privoxy is one package we are using for improving the web browsing > > experience. Toward that end, we've added all the http-everywhere and > > easyprivacy rules to our version of privoxy to upgrade connections to > > ssl and to block web tracking. > > > > The code is at https://github.com/jvasile/freedombox-privoxy and we're > > testing it now for release later this month. > > Great. So far I only cloned the repository and skimmed over it, > but I'll try to have a closer look in the next days. A converter > from Adblock Plus to Privoxy files has been on our TODO list for > a while now. My converter is somewhat primitive and lossy. I'm operating on the principle that some conversion is better than none. It still needs work and some testing. > > > In order to do the https-everywhere ruleset, I had to add a few lines to > > privoxy so that redirects could contain multiple regex substitutions. > > It's not obvious to me why a https-everywhere rule set would require > multiple regex substitutions. Could you give an example? Take this https-everywhere rule: <!-- blogg.binero.se lacks ssl support. --> <ruleset name="Binero.se"> <target host="binero.se" /> <rule from="^http://binero\.se/" to="https://www.binero.se/"/> <rule from="^http://support\.binero\.se/" to="https://support.binero.se/"/> <rule from="^http://order\.binero\.se/" to="https://order.binero.se/"/> <rule from="^http://www\.binero\.se/" to="https://www.binero.se/"/> </ruleset> If a request comes in matching binero.se, it needs to be checked against all the regexes, but if I do several +redirect lines, privoxy sees each one as trumping the earlier ones and only applies the last one. For some of the rules like this, I can manually edit the ruleset to fix the problem, but I don't see a way to do it programmatically. > > > I'd like to offer changes to Debian and also to the privoxy project, but > > I'm not sure if you're interested. It's a lot of additional rules, but > > I think it would be cool if privoxy offered at least some of these > > features to all of Debian or to everybody in the world, not just our own > > project. > > Given that the https-everywhere and "easyprivacy" rules are actively > maintained elsewhere and likely frequently updated, while there are > usually several months between Privoxy releases, I don't think including > translated rules in Privoxy releases would be a good idea. >From a convenience perspective, my guess is that many users would probably like rules included by default, even if they are a couple months out of date. But I take your point. > > Allowing Privoxy users to leverage those rule sources by either > downloading already transformed action files or by converting the > rules themselves makes a lot of sense, though. > > If this really requires modifications to Privoxy itself, I wouldn't > mind helping with their implementation or reviewing patches. > > > Please let me know the best way to offer changes to the Debian package > > or to the privoxy project. Below is the code for multiple regexes in > > redirects. It is only needed to make https-everywhere work, not > > easyprivacy. I'm open to adjusting my work for better fit with yours. > > The patch has a couple of issues: > > 1) Privoxy is usually built with threading support so all > the functions it uses need to be either thread-safe or > protected with a mutex lock. Using strtok() the way you > do is likely to cause incorrect redirects under load, > unless you disable threading support. Would switching to strtok_r do it? > > You should be able to confirm this with Privoxy-Regression-Test: > https://sourceforge.net/tracker/?func=detail&aid=3429848&group_id=11118&atid=311118 Here is the result of the test: 2012-01-16 21:20:39: Asking Privoxy for the number of action files available ... 2012-01-16 21:20:39: Gathering regression tests from 5 action file(s) delivered by Privoxy. 2012-01-16 21:20:39: Executing regression tests ... 2012-01-16 21:20:44: Executed 298 regression tests. Skipped 14. 298 successes, 0 failures. > > BTW, did you already consider letting your rules converters > generate tests for Privoxy-Regression-Test? If there's > enough information in the source to do this, it should > make testing the translated action files a lot easier. No, but I will look into this. > > 2) pcrs_command is declared const char * so passing it to > strtok(), which at least on my FreeBSD system will modify > the data while splitting it, is an API violation and should > (at least) cause a compiler complaint. Good point. I'll copy the string and operate on the copy. > > 3) Assuming redirecting a request based on multiple pcrs > commands is really necessary, I think a more elegant > solution would be to allow multiple matching redirect{} > actions, each with a single pcrs command, or to add a > redirect-filter{} action that works similar to the > client-header-filter{} action and only references a > "redirect filter" that is defined in a filter file. > > Doing the latter would have the added benefit that > the "redirect filter" only needs to be compiled once > when loading the filter file and not for every request > the action applies to. My approach is definitely a bit of a kludge. Your makes makes more sense. I will look into your approach. In the mean time, I've pushed a new filters.c to git per your suggestions. It is below. Thanks much, James #! /bin/sh /usr/share/dpatch/dpatch-run ## 97_filters.c.dpatch by James Vasile <[email protected]> --- Debian/freedombox-privoxy-3.0.19-1-1/filters.c 2012-01-16 21:25:20.000000000 -0500 +++ privoxy/filters.c 2012-01-16 21:25:14.000000000 -0500 @@ -1016,9 +1016,10 @@ * * Function : rewrite_url * - * Description : Rewrites a URL with a single pcrs command - * and returns the result if it differs from the - * original and isn't obviously invalid. + * Description : Rewrites a URL with one or more pcrs commands and + * returns the result if it differs from the original + * and isn't obviously invalid. Separate pcrs commands + * with tabs. * * Parameters : * 1 : old_url = URL to rewrite. @@ -1032,12 +1033,39 @@ char *rewrite_url(char *old_url, const char *pcrs_command) { char *new_url = NULL; - int hits; + int hits=0; assert(old_url); assert(pcrs_command); - new_url = pcrs_execute_single_command(old_url, pcrs_command, &hits); + // Copy pcrs_command so we don't change it in the strtok_r + char *copy_command = malloc(strlen(pcrs_command) + 1); + if (copy_command==NULL) + { + log_error(LOG_LEVEL_ERROR, "Couldn't allocate memory to test for redirect."); + return old_url; + } + strcpy(copy_command, pcrs_command); + + char *saveptr; + char * pch; + int h=0; + pch = strtok_r(copy_command, "\t", &saveptr); + char *subject; + subject = old_url; + while (pch != NULL) + { + new_url = pcrs_execute_single_command(subject, pch, &h); + pch = strtok_r(NULL, "\t", &saveptr); + hits += h; + + if (subject != old_url) + { + freez(subject); + } + subject = new_url; + } + freez(copy_command); if (hits == 0) { ------------------------------------------------------------------------------ Keep Your Developer Skills Current with LearnDevNow! The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now! http://p.sf.net/sfu/learndevnow-d2d