You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2016/03/01 15:37:46 UTC
svn commit: r1733080 - in /tomcat/trunk:
java/org/apache/tomcat/util/buf/UriUtil.java webapps/docs/changelog.xml
Author: markt
Date: Tue Mar 1 14:37:46 2016
New Revision: 1733080
URL: http://svn.apache.org/viewvc?rev=1733080&view=rev
Log:
Expand the fix for BZ 59001 to cover the special sequences used in Tomcat's custom jar:war: URL
Modified:
tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java?rev=1733080&r1=1733079&r2=1733080&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java Tue Mar 1 14:37:46 2016
@@ -106,6 +106,9 @@ public final class UriUtil {
private static String makeSafeForJarUrl(String input) {
// Since "!/" has a special meaning in a JAR URL, make sure that the
// sequence is properly escaped if present.
- return input.replaceAll("!/", "%21/");
+ String tmp = input.replaceAll("!/", "%21/");
+ // Tomcat's custom jar:war: URL handling treats */ and ^/ as special
+ tmp = tmp.replaceAll("^/", "%5e/");
+ return tmp.replaceAll("\\*/", "%2a/");
}
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1733080&r1=1733079&r2=1733080&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Tue Mar 1 14:37:46 2016
@@ -107,6 +107,10 @@
<bug>59001</bug>: Correctly handle the case when Tomcat is installed on
a path where one of the segments ends in an exclamation mark. (markt)
</fix>
+ <fix>
+ Expand the fix for <bug>59001</bug> to cover the special sequences used
+ in Tomcat's custom jar:war: URLs. (markt)
+ </fix>
<update>
Switch to the web application class loader to the
<code>ParallelWebappClassLoader</code> by default. (markt)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1733080 - in /tomcat/trunk: java/org/apache/tomcat/util/buf/UriUtil.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
2016-03-01 15:57 GMT+01:00 Martin Grigorov <mg...@apache.org>:
> Hi Mark,
>
> On Tue, Mar 1, 2016 at 3:37 PM, <ma...@apache.org> wrote:
>
> > Author: markt
> > Date: Tue Mar 1 14:37:46 2016
> > New Revision: 1733080
> >
> > URL: http://svn.apache.org/viewvc?rev=1733080&view=rev
> > Log:
> > Expand the fix for BZ 59001 to cover the special sequences used in
> > Tomcat's custom jar:war: URL
> >
> > Modified:
> > tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
> > tomcat/trunk/webapps/docs/changelog.xml
> >
> > Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
> > URL:
> >
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java?rev=1733080&r1=1733079&r2=1733080&view=diff
> >
> >
> ==============================================================================
> > --- tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java (original)
> > +++ tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java Tue Mar 1
> > 14:37:46 2016
> > @@ -106,6 +106,9 @@ public final class UriUtil {
> > private static String makeSafeForJarUrl(String input) {
> >
>
>
>
> > // Since "!/" has a special meaning in a JAR URL, make sure that
> > the
> > // sequence is properly escaped if present.
> > - return input.replaceAll("!/", "%21/");
> > + String tmp = input.replaceAll("!/", "%21/");
> > + // Tomcat's custom jar:war: URL handling treats */ and ^/ as
> > special
> > + tmp = tmp.replaceAll("^/", "%5e/");
> > + return tmp.replaceAll("\\*/", "%2a/");
> >
>
> How often this method is expected to be called? I guess at least once per
> request.
>
No, it's supposed to be an init "scan" method, not a once per request.
OTOH, sometimes there are like thousands of jars, so if it's really that
slow ...
Rémy
>
> My concern is about the performance of String#replaceAll. It uses Regex and
> is slower than custom solutions like
>
> https://github.com/apache/wicket/blob/ffa34c6bfbd2ccd8340e23ff1601edd3e0e941d6/wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java#L748
>
> When I don't have access to such util methods in the classpath then I
> prefer to pre-compile the Pattern as a constant and just match on it:
> e.g. PERCENT_21_PATTERN.matcher(input).replaceAll("%21/")
>
> Additionally I have the feeling that 'tmp.replaceAll("^/", "%5e/");' won't
> behave as desired. I think it would match for any String that starts with a
> slash because of '^'. You may need to Pattern.quote() it.
>
>
> > }
> > }
> >
> > Modified: tomcat/trunk/webapps/docs/changelog.xml
> > URL:
> >
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1733080&r1=1733079&r2=1733080&view=diff
> >
> >
> ==============================================================================
> > --- tomcat/trunk/webapps/docs/changelog.xml (original)
> > +++ tomcat/trunk/webapps/docs/changelog.xml Tue Mar 1 14:37:46 2016
> > @@ -107,6 +107,10 @@
> > <bug>59001</bug>: Correctly handle the case when Tomcat is
> > installed on
> > a path where one of the segments ends in an exclamation mark.
> > (markt)
> > </fix>
> > + <fix>
> > + Expand the fix for <bug>59001</bug> to cover the special
> > sequences used
> > + in Tomcat's custom jar:war: URLs. (markt)
> > + </fix>
> > <update>
> > Switch to the web application class loader to the
> > <code>ParallelWebappClassLoader</code> by default. (markt)
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
>
Re: svn commit: r1733080 - in /tomcat/trunk: java/org/apache/tomcat/util/buf/UriUtil.java
webapps/docs/changelog.xml
Posted by Martin Grigorov <mg...@apache.org>.
Hi Mark,
On Tue, Mar 1, 2016 at 6:53 PM, Mark Thomas <ma...@apache.org> wrote:
> On 01/03/2016 14:57, Martin Grigorov wrote:
> > On Tue, Mar 1, 2016 at 3:37 PM, <ma...@apache.org> wrote:
> >
> >> Author: markt
> >> Date: Tue Mar 1 14:37:46 2016
> >> New Revision: 1733080
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1733080&view=rev
> >> Log:
> >> Expand the fix for BZ 59001 to cover the special sequences used in
> >> Tomcat's custom jar:war: URL
>
> <snip/>
>
> > How often this method is expected to be called? I guess at least once per
> > request.
>
> That is not correct. It is generally called during web application
> start. I'd typically expect it to be called twice per JAR plus a few
> more times per web application for configuration files (depending on
> Host configuration).
>
OK. If the method is not called very often then it is not a big deal.
>
> > My concern is about the performance of String#replaceAll. It uses Regex
> and
> > is slower than custom solutions like
> >
> https://github.com/apache/wicket/blob/ffa34c6bfbd2ccd8340e23ff1601edd3e0e941d6/wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java#L748
> >
> > When I don't have access to such util methods in the classpath then I
> > prefer to pre-compile the Pattern as a constant and just match on it:
> > e.g. PERCENT_21_PATTERN.matcher(input).replaceAll("%21/")
>
> Given how infrequently this code will be called, when it will be called
> and the overhead of JAR handling overall compared to the contribution of
> these calls I don't think a custom replaceAll() is necessary (although
> if user feedback is different for some use cases we can always revisit
> that).
>
> The pre-compiled Pattern approach might be worth looking at. I'll see if
> I can put together a simple benchmark and add it to the unit tests.
>
I've seen the following commit with the compiled Pattern!
>
> > Additionally I have the feeling that 'tmp.replaceAll("^/", "%5e/");'
> won't
> > behave as desired. I think it would match for any String that starts
> with a
> > slash because of '^'. You may need to Pattern.quote() it.
>
> It does behave as intended. There was a test case that checked that that
> wasn't checked in with the original commit.
>
Are you sure? ;-)
public static void main(String[] args) {
System.err.println("aaa^/bbb".replaceAll("^/", "C"));
System.err.println("aaa^/bbb".replaceAll("\\^/", "C"));
}
Executing this prints:
aaa^/bbb
aaaCbbb
In the following commit this is escaped and works correctly!
Thanks!
>
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: svn commit: r1733080 - in /tomcat/trunk:
java/org/apache/tomcat/util/buf/UriUtil.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 01/03/2016 14:57, Martin Grigorov wrote:
> On Tue, Mar 1, 2016 at 3:37 PM, <ma...@apache.org> wrote:
>
>> Author: markt
>> Date: Tue Mar 1 14:37:46 2016
>> New Revision: 1733080
>>
>> URL: http://svn.apache.org/viewvc?rev=1733080&view=rev
>> Log:
>> Expand the fix for BZ 59001 to cover the special sequences used in
>> Tomcat's custom jar:war: URL
<snip/>
> How often this method is expected to be called? I guess at least once per
> request.
That is not correct. It is generally called during web application
start. I'd typically expect it to be called twice per JAR plus a few
more times per web application for configuration files (depending on
Host configuration).
> My concern is about the performance of String#replaceAll. It uses Regex and
> is slower than custom solutions like
> https://github.com/apache/wicket/blob/ffa34c6bfbd2ccd8340e23ff1601edd3e0e941d6/wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java#L748
>
> When I don't have access to such util methods in the classpath then I
> prefer to pre-compile the Pattern as a constant and just match on it:
> e.g. PERCENT_21_PATTERN.matcher(input).replaceAll("%21/")
Given how infrequently this code will be called, when it will be called
and the overhead of JAR handling overall compared to the contribution of
these calls I don't think a custom replaceAll() is necessary (although
if user feedback is different for some use cases we can always revisit
that).
The pre-compiled Pattern approach might be worth looking at. I'll see if
I can put together a simple benchmark and add it to the unit tests.
> Additionally I have the feeling that 'tmp.replaceAll("^/", "%5e/");' won't
> behave as desired. I think it would match for any String that starts with a
> slash because of '^'. You may need to Pattern.quote() it.
It does behave as intended. There was a test case that checked that that
wasn't checked in with the original commit.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1733080 - in /tomcat/trunk: java/org/apache/tomcat/util/buf/UriUtil.java
webapps/docs/changelog.xml
Posted by Martin Grigorov <mg...@apache.org>.
Hi Mark,
On Tue, Mar 1, 2016 at 3:37 PM, <ma...@apache.org> wrote:
> Author: markt
> Date: Tue Mar 1 14:37:46 2016
> New Revision: 1733080
>
> URL: http://svn.apache.org/viewvc?rev=1733080&view=rev
> Log:
> Expand the fix for BZ 59001 to cover the special sequences used in
> Tomcat's custom jar:war: URL
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java?rev=1733080&r1=1733079&r2=1733080&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/buf/UriUtil.java Tue Mar 1
> 14:37:46 2016
> @@ -106,6 +106,9 @@ public final class UriUtil {
> private static String makeSafeForJarUrl(String input) {
>
> // Since "!/" has a special meaning in a JAR URL, make sure that
> the
> // sequence is properly escaped if present.
> - return input.replaceAll("!/", "%21/");
> + String tmp = input.replaceAll("!/", "%21/");
> + // Tomcat's custom jar:war: URL handling treats */ and ^/ as
> special
> + tmp = tmp.replaceAll("^/", "%5e/");
> + return tmp.replaceAll("\\*/", "%2a/");
>
How often this method is expected to be called? I guess at least once per
request.
My concern is about the performance of String#replaceAll. It uses Regex and
is slower than custom solutions like
https://github.com/apache/wicket/blob/ffa34c6bfbd2ccd8340e23ff1601edd3e0e941d6/wicket-util/src/main/java/org/apache/wicket/util/string/Strings.java#L748
When I don't have access to such util methods in the classpath then I
prefer to pre-compile the Pattern as a constant and just match on it:
e.g. PERCENT_21_PATTERN.matcher(input).replaceAll("%21/")
Additionally I have the feeling that 'tmp.replaceAll("^/", "%5e/");' won't
behave as desired. I think it would match for any String that starts with a
slash because of '^'. You may need to Pattern.quote() it.
> }
> }
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1733080&r1=1733079&r2=1733080&view=diff
>
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Tue Mar 1 14:37:46 2016
> @@ -107,6 +107,10 @@
> <bug>59001</bug>: Correctly handle the case when Tomcat is
> installed on
> a path where one of the segments ends in an exclamation mark.
> (markt)
> </fix>
> + <fix>
> + Expand the fix for <bug>59001</bug> to cover the special
> sequences used
> + in Tomcat's custom jar:war: URLs. (markt)
> + </fix>
> <update>
> Switch to the web application class loader to the
> <code>ParallelWebappClassLoader</code> by default. (markt)
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>