ezelkow1 commented on code in PR #13776:
URL: https://github.com/apache/trafficserver/pull/13776#discussion_r4162971067
##########
src/iocore/cache/HttpTransactCache.cc:
##########
@@ -86,10 +86,10 @@ do_strings_match_strongly(const char *raw_tag_field, int
raw_tag_field_len, cons
const char *etag_start;
int n, etag_length;
- // Can never match a weak tag with a strong compare
- if ((raw_tag_field_len >= 2) && (raw_tag_field[0] == 'W' && raw_tag_field[1]
== '/')) {
- return false;
- }
+ // A missing or weak tag never matches a specific tag under a strong
compare, but "*" matches any
+ // current representation.
+ bool const only_star = (raw_tag_field_len <= 0) || (raw_tag_field_len >= 2
&& raw_tag_field[0] == 'W' && raw_tag_field[1] == '/');
+
// Find the unalterated tag
etag_start = find_etag(raw_tag_field, raw_tag_field_len, &etag_length);
Review Comment:
In C++, adding 0 to a null pointer is well-defined and yields a null pointer
([expr.add]/4.1, and null − null is 0 per /5), unlike C. `find_etag()` doesn't
dereference in that case (its `memchr` is guarded by `etag_start < etag_end`),
and the pre-existing code already called it with an empty view when the stored
response had no ETag (for both If-Match and If-Range).
##########
src/iocore/cache/HttpTransactCache.cc:
##########
@@ -1384,12 +1388,8 @@
HttpTransactCache::match_response_to_request_conditionals(HTTPHdr *request, HTTP
// If-Match: must match strongly //
if (request->presence(MIME_PRESENCE_IF_MATCH)) {
- auto
raw_etags{response->value_get(static_cast<std::string_view>(MIME_FIELD_ETAG))};
- std::string_view comma_sep_tag_list{};
-
- if (!raw_etags.empty()) {
- comma_sep_tag_list =
request->value_get(static_cast<std::string_view>(MIME_FIELD_IF_MATCH));
- }
+ auto
raw_etags{response->value_get(static_cast<std::string_view>(MIME_FIELD_ETAG))};
+ auto
comma_sep_tag_list{request->value_get(static_cast<std::string_view>(MIME_FIELD_IF_MATCH))};
if (do_strings_match_strongly(raw_etags.data(),
static_cast<int>(raw_etags.length()), comma_sep_tag_list.data(),
Review Comment:
On a match the function returns the cached response's status, so a
negative-cached 404 with `If-Match: *` is answered with the cached 404, not a
success. Verified with negative caching on: before this change it returned 412,
now 404. RFC 9110 §13.2.1 requires preconditions to be ignored when the
response without them would be non-2xx, so 404 is correct and 412 was not. A
specific-tag If-Match on a cached 404 still returns 412, which predates this PR
(ATS doesn't apply §13.2.1 to cached non-2xx responses in general) and is
outside its scope.
This also keeps conditionals consistent with negative caching: while ATS is
deliberately serving a cached 404 (for example to shield the origin),
`If-Match: *` now gets the same 404 a plain GET gets rather than a 412, and
either way the request is answered from cache without contacting the origin. A
negative-cache regression case (asserting the 404, that the origin isn't
contacted, and documenting the specific-tag 412) will follow in the next commit.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
