apache
Pass url_len when emplacing in Http2CommonSession::_h2_pushed_urls
The signature of the function in question is
void
Http2CommonSession::add_url_to_pushed_table(const char *url, int url_len)
The given url is used when emplacing/inserting to _h2_pushed_urls collection but the given url_len is not used and this may change the actually inserted URL because it'll search for the \0 terminator instead of inserting only url_len characters/bytes.
And the \0 terminator may not even be present in some cases (the TSHttpTxnServerPush calls the above function i.e. it may be called by different plugins).
I've opened this issue about this and it resulted in this pull request.
Note that the problem with url_len is reported by the compiler if/when the warning for unused parameters is enabled.
fixes https://github.com/apache/trafficserver/issues/11375
Good find! It looks like add_url_to_pushed_table is only used in TSHttpTxnServerPush (in src/api/InkAPI.cc). As a part of this PR, I suggest you add sdk_assert(url && url_len > 0) in TSHttpTxnServerPush. Some TS API functions that accept strings with lengths work like this: https://github.com/apache/trafficserver/blob/4f74b0222b5f6b77cf7a39a56bc8807595c35bc9/src/api/InkAPI.cc#L1292 So, someone might assume they can pass -1 as the length.
My bad. I'm not very accustomed to this pattern yet.
Do you think I should add the sdk_assert in TSHttpTxnServerPush or I should add the check with the if (len < 0) len = strlen(...)?
I'm asking because the TSHttpTxnServerPush function calls
url_obj.parse(url, url_len);
and the url_obj also does this check internally i.e. it's prepared for -1 length and thus maybe the it's expected that TSHttpTxnServerPush can also take -1 length and should be able to handle it instead of asserting.
I'm not sure what the consensus is about this. I asked in the slack channel: https://the-asf.slack.com/archives/CHQ1FJ9EG/p1716395850701069
Masakazu prefers TSHttpTxnServerPush support negative URL length by calling strlen on the URL pointer if the length is negative.
Updated the pull request.
Overall the situation with this "maybe-negative-length" pattern is a bit confusing (at least for me). I mean, if there this usage:
void some_exported_API(const char* str, int len)
{
internal_fun_1(str, len);
internal_fun_2(str, len);
internal_fun_3(str, len);
}
Is the check of if (len < 0) len = strlen(str) supposed to happen at the highest level i.e. in the some_exported_API or every internal function is supposed to have this check (but then the if-check or the call to strlen will be potentially repeated multiple times).
Or the internal functions are just supposed to assert the len >= 0? But then why just don't get it as size_t.
Anyhow, the these are just my thoughts. It's what it's.
