Return string view from HTTPHdr methods #12091
Conversation
hnakamur
force-pushed
the
return_string_view_from_HTTPHdr_methods
branch
from
448ecec to
90a30ce
Compare
| memccpy(hname, vector.get(i)->request_get()->host_get(&hlen), 0, 500); | ||
| auto host{vector.get(i)->request_get()->host_get()}; | ||
| hlen = static_cast<int>(host.length()); | ||
| memccpy(hname, host.data(), 0, 500); |
There was a problem hiding this comment.
This is concerning here since string_view.data() isn't necessarily null terminated, but this copy is looking for one. This should be memcpy or strncpy (but still add the termination). Ideally hname would also be a string_view, but I'd have to dig further to see if that is possible without more refactoring.
src/proxy/http/HttpSM.cc
Outdated
| if (host_sni_policy == 2) { | ||
| swoc::bwprint(error_bw_buffer, "No SNI for TLS request: connecting to {} for host='{}', returning a 403", | ||
| t_state.client_info.dst_addr, std::string_view{host_name, static_cast<size_t>(host_len)}); | ||
| t_state.client_info.dst_addr, host_name.data()); |
There was a problem hiding this comment.
You should drop .data() for these fmt style prints. They support sv directly.
There was a problem hiding this comment.
Thanks! Fixed in efb8ee8
src/proxy/http/HttpSM.cc
Outdated
| char const *ptr = t_state.hdr_info.server_request.host_get(&len); | ||
| zret.assign(ptr, len); | ||
| auto host{t_state.hdr_info.server_request.host_get()}; | ||
| zret.assign(host.data(), static_cast<int>(host.length())); |
There was a problem hiding this comment.
I think this could be: zret = t_state.hdr_info.server_request.host_get()
There was a problem hiding this comment.
Thanks! Fixed in 9e31e96
src/proxy/http/HttpTransact.cc
Outdated
|
|
||
| if (is_localhost(host, host_len)) { | ||
| if (is_localhost(host.data(), static_cast<int>(host.length()))) { |
There was a problem hiding this comment.
is_localhost could take a string_view instead. This is the only place it is called.
There was a problem hiding this comment.
Thanks! Fixed in 266bea6
There was a problem hiding this comment.
There is one test failure in https://ci.trafficserver.apache.org/job/Github_Builds/job/ubuntu/5645/console
RPRINT HostDBProcessor: HostDBRecord hostname=127.0.0.1
RPRINT HostDBProcessor: hostdbinfo RR0 ip=127.0.0.1
RPRINT HostDBProcessor: HostDBTestRR: 0 outstanding 5 success 1 failure
REGRESSION_RESULT HostDBProcessor: FAILED
However it is not reproduced on my Ubuntu machine. I ran the following commands:
cmake -B build --preset ci-ubuntu
cmake --build build -j -v
cmake --install build
/tmp/ats/bin/traffic_server -K -k -R 1
All tests passed.
RPRINT HostDBProcessor: HostDBRecord hostname=127.0.0.1 RPRINT HostDBProcessor: hostdbinfo RR0 ip=127.0.0.1 RPRINT HostDBProcessor: HostDBTestRR: 0 outstanding 6 success 0 failure REGRESSION_RESULT HostDBProcessor: PASSED
...(snip)...
REGRESSION_TEST DONE: PASSED
No description provided.