You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/07/20 14:57:03 UTC
[GitHub] [tomcat] stokito opened a new pull request #325: Simplify ETag check
stokito opened a new pull request #325:
URL: https://github.com/apache/tomcat/pull/325
The If-None-Match header many have multiple ETags https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.26
Even if Tomcat returns a single ETag clients may want to send few of them.
Since ETags are always quoted the easiest way to check is just to check a substring.
This will reduce amount of memory consumed by full ETag parsing.
A command to check:
curl -vv http://localhost:8080/tomcat.png -H 'If-None-Match: W/"5103-1595246577000", "etag2"'
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661356032
I found related ticket https://bz.apache.org/bugzilla/show_bug.cgi?id=64265
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
reschke commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463584147
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
Review comment:
I *believe* just doing "contains()" will yield incorrect results if the field value is malformed, such as `WW/"1"`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463648280
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
Review comment:
GIGO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463555063
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
Review comment:
it is cheap but rare in practice. Browsers never sends asterisk, it is only needed for some tricky WebDav put requests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666445794
I added another two commits with performance optimization. Given the nature of ETags that Tomcat generates itself I made some priority to check ETag:
1. Full match (what we normally expect)
2. Weak match
3. Asterisk match (not sure who ever used it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463568249
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
Review comment:
Yes, it may look not so obvious for those who never used such approach. Still it makes a lot of sense to use this.
In 99% (if not all 100) cases the If-None-Match will be just a single ETag that Tomcat generated itself.
But create an instance of the `StringTokenizer` allocates 160 bytes, and each a new `nextToken()` call takes about 88 bytes for etag like `W/"1047-1578315296666"`.
The contains() call have no any allocations and it is optimized intrinsic function in JVM
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600
At least twice fast because creates less memory garbage. For two comma separated ETags may work 5 times faster.
Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661164935
You are citing the spec very selectively (hint: there are strong and weak comparion functions).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf closed pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
markt-asf closed pull request #325:
URL: https://github.com/apache/tomcat/pull/325
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661111650
Thank you, good points! I checked spec https://tools.ietf.org/html/rfc7232#section-2.3.2 and it looks like we still fine here because Tomcat in fact should do a weak comparison.
|If-None-Match | Real resource's ETag | Match |
|--------------------|-----------------------------|---------------------|
|`W/"1"` | `W/"1"` | Yes |
|`"1"` | `W/"1"` | No. |
|`W/"1"` | `"1"` | Yes, as in spec |
|`"1"` | `"1"` | Yes |
|`W/"1", "1"` | `"1"` | Yes |
|`W/"1", "2"` | `W/"2"` | No |
A Comma inside of ETag won’t be a problem.
In fact this is simple, fast and straightforward algorithm.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666382555
Ok, so Tomcat should make a weak matching.
I changed the description of PR and added commits. Now this is a bug fix and simplification is only in last commit.
You can merge everything except of the last commit d4831ba.
Since logic of matching now even more complicated I updated the benchmark:
https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
But more important is that it just looks simpler.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463516693
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
+ }
+
+ /**
+ * RFC 7232 requires strong comparison for If-Match
+ */
+ private boolean matchByEtagStrong(String headerValue, String eTag) {
+ // BZ 64265: Default servlet uses weak matching so we strip any leading "W/" and
+ // then compare using equals
+ String resourceEtag = weakEtagToStrong(eTag);
+ StringTokenizer commaTokenizer = new StringTokenizer(headerValue, ",");
Review comment:
You have critisized this block, but remained at the split? Note that a comma can also appear in an ETag value.
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
Review comment:
Asterisk match is the cheapest one and should go first as described in the RFC.
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
Review comment:
I am not a huge a fan of something like this, I'd rather prefer a real split here.
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
+ }
+
+ /**
+ * RFC 7232 requires strong comparison for If-Match
+ */
+ private boolean matchByEtagStrong(String headerValue, String eTag) {
+ // BZ 64265: Default servlet uses weak matching so we strip any leading "W/" and
+ // then compare using equals
+ String resourceEtag = weakEtagToStrong(eTag);
Review comment:
This does not feel right because if the ETag is weak, the comparison can never succeed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463563011
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
+ }
+
+ /**
+ * RFC 7232 requires strong comparison for If-Match
+ */
+ private boolean matchByEtagStrong(String headerValue, String eTag) {
+ // BZ 64265: Default servlet uses weak matching so we strip any leading "W/" and
+ // then compare using equals
+ String resourceEtag = weakEtagToStrong(eTag);
Review comment:
the tomcat's own Weak etags will be converted to strong i.e. the W/ is remored inside weakEtagToStrong(). So it's fine.
You can run a test that checks it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
markt-asf commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-672005180
Simplifying the matching is the wrong approach. The matching needs to be tightened up to be more spec compliant. A change to that effect has now been implemented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600
At least twice fast because creates less memory garbage.
Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661356032
a related ticket https://bz.apache.org/bugzilla/show_bug.cgi?id=64265
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661111650
Thank you, good points! I checked spec https://tools.ietf.org/html/rfc7232#section-2.3.2 and it looks like we still fine here because Tomcat in fact should do a weak comparison.
|If-None-Match | Real resource's ETag | Match |
|--------------------|-----------------------------|---------------------|
|`W/"1"` | `W/"1"` | Yes |
|`"1"` | `W/"1"` | No. |
|`W/"1"` | `"1"` | Yes, as in spec |
|`"1"` | `"1"` | Yes |
|`W/"1", "1"` | `"1"` | Yes |
|`W/"1", "2"` | `W/"2"` | No |
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661279813
> But the change improves performance a lot.
Can you quantify that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600
At least twice fast because and creates less memory garbage.
Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r468504398
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
Review comment:
Garbage in, garbage out is not an acceptable way to handle HTTP headers. Failure to reject malformed HTTP headers from clients can lead to security issues - typically request smuggling - when a proxy takes a different approaching to allowing the invalid header to the back-end server although in this instance the proxy would need to do something pretty unusual.
The right solution here is to implement RFC 7232 compliant parsing of `entity-tag` in `org.apache.tomcat.util.http.parser`
Tomcat has been tightening up the parsing of HTTP headers over time, generally improving things as parsing issues are raised with each header. It appears that now is the time to address `entity-tag`. I also note that we should make the use of weak comparison for `If-Match` configurable. Users that extend the Default servlet to provide strong ETags (or use a custom resource implementation) will almost certainly want to use a strong comparison here.
I'll start work on a suitable parser.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
reschke commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463563546
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
Review comment:
PUT with "If-None-Match: *" is standard HTTP, not "tricky WebDAV". It means: "fail the request if the request would overwrite an existing representation". (see https://greenbytes.de/tech/webdav/rfc7232.html#rfc.section.3.2.p.5)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661607221
> At least twice fast because creates less memory garbage
And this is measurable in practice., not in an isolated unit test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666445794
I added another two commits with performance optimization. Given the nature of ETags that Tomcat generates itself I made some priority to check ETag:
1. Strict comparision (what we normally expect)
2. Weak comparision
3. Asterisk (not sure who ever used it)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666645463
OMG you right, they differ:
An origin server MUST use the strong comparison function when
comparing entity-tags for If-Match (Section 2.3.2), since the client
intends this precondition to prevent the method from being applied if
there have been any changes to the representation data.
A recipient MUST use the weak comparison function when comparing
entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
can be used for cache validation even if there have been changes to
the representation data.
But what is funny is that in Tomcat on the contrary `If-Match` is *weak* while `If-None-Match` is *strong*.
I'll fix that and update the PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463554358
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
+ }
+
+ /**
+ * RFC 7232 requires strong comparison for If-Match
+ */
+ private boolean matchByEtagStrong(String headerValue, String eTag) {
+ // BZ 64265: Default servlet uses weak matching so we strip any leading "W/" and
+ // then compare using equals
+ String resourceEtag = weakEtagToStrong(eTag);
+ StringTokenizer commaTokenizer = new StringTokenizer(headerValue, ",");
Review comment:
yes, because contains() can't be used for the strict comparison. We can do a similar custom function (i.e. without creation of heavy StringTokenizer) but TBH the `If-Match` is used rarely and as far I understood it was intended for WebDav's PUT requests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke edited a comment on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
reschke edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661164935
You are citing the spec very selectively (hint: there are strong and weak comparison functions).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661232860
Yeah, that's why I said "in fact should do". This is not a big deal while Tomcat generates those ETag itself. I'm not even sure that there is any clients who sends multiple ETags. But the change improves performance a lot.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666732691
@michael-o now I fixed, please review
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661298600
At least triple fast because creates less memory garbage. For two comma separated ETags may work 10 times faster.
Here is a benchmark with results https://gist.github.com/stokito/a82eed1aef6ad965e2a279825f1c3420
I tested with force GC between tests to compare just CPU time. Note that this comparison performed for each request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
reschke commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463564111
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
+ }
+
+ /**
+ * RFC 7232 requires strong comparison for If-Match
+ */
+ private boolean matchByEtagStrong(String headerValue, String eTag) {
+ // BZ 64265: Default servlet uses weak matching so we strip any leading "W/" and
+ // then compare using equals
+ String resourceEtag = weakEtagToStrong(eTag);
+ StringTokenizer commaTokenizer = new StringTokenizer(headerValue, ",");
Review comment:
PUT with conditional header fields is standard base HTTP (yes, used in WebDAV, but in other scenarios as well).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463562124
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
Review comment:
maybe I missed something, but this is complete with spec. The only diff is that we'll check for full ETags firts
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666737304
Will do tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] reschke commented on pull request #325: Simplify ETag check
Posted by GitBox <gi...@apache.org>.
reschke commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-661095956
The suggested change will cause incorrect behaviour, for instance
"foobar"
would match
W/"foobar"
That said, the existing code is broken as well, as it will fail if an ETag contains a comma.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito edited a comment on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito edited a comment on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666645463
OMG you right, they differ:
An origin server MUST use the strong comparison function when
comparing entity-tags for If-Match (Section 2.3.2), since the client
intends this precondition to prevent the method from being applied if
there have been any changes to the representation data.
A recipient MUST use the weak comparison function when comparing
entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
can be used for cache validation even if there have been changes to
the representation data.
But what is funny is that in Tomcat on the contrary `If-Match` is *weak* while `If-None-Match` is *strong*.
I'll fix that and update the PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] stokito commented on pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
stokito commented on pull request #325:
URL: https://github.com/apache/tomcat/pull/325#issuecomment-666645463
OMG you right, they differ:
An origin server MUST use the strong comparison function when
comparing entity-tags for If-Match (Section 2.3.2), since the client
intends this precondition to prevent the method from being applied if
there have been any changes to the representation data.
A recipient MUST use the weak comparison function when comparing
entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
can be used for cache validation even if there have been changes to
the representation data.
But what is funny is that in Tomcat on the contrary `If-Match` is *weak* while `If-None-Match` is *strong*.
I'll fix that and update the PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] michael-o commented on a change in pull request #325: BZ 64265 Fix and simplify weak ETag matching
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #325:
URL: https://github.com/apache/tomcat/pull/325#discussion_r463559607
##########
File path: java/org/apache/catalina/servlets/DefaultServlet.java
##########
@@ -2611,6 +2574,44 @@ private PrecompressedResource(WebResource resource, CompressionFormat format) {
}
}
+ /**
+ * RFC 7232 requires weak comparison for If-None-Match
+ */
+ private boolean matchByEtagWeak(String headerValue, String eTag) {
+ // Match W/"1" and W/"1"
+ if (headerValue.contains(eTag)) {
+ return true;
+ }
+ // Match W/"1" and "1"
+ String resourceEtag = weakEtagToStrong(eTag);
+ if (headerValue.contains(resourceEtag)) {
+ return true;
+ }
+ // asterisk checked last as rarely used
+ return headerValue.equals("*");
Review comment:
That's right, we use it for updates via REST. But still, the implementation should be complete.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org