[BUG] Stop reading past a string_view that is not NUL terminated

[thc1006]

nostd::string_view::data() points at the first character of a view, not at a C string. Anything that reads until a NUL can therefore run past the end of what the caller offered. Four places do that.

The one that changes an answer

HttpSslOptions decides use_ssl from the url it is given:

if (strncmp(url.data(), "https:", 6) == 0)

strncmp reads up to six bytes whatever the view's length is. A view holding the five characters https over a buffer that continues with a colon matches, and the connection turns on TLS the caller never asked for. Measured, with the view length being the only difference:

  view    = https (len 5)
  old_way = 1   reads buf[5] past the view
  new_way = 0   honours the view length

The replacement compares within the view, and <cstring> was there for that one call so it goes too.

The three that read too far

BuildResponseLogMessage in the Elasticsearch and the OTLP HTTP exporters streams header names and values through the const char * overload:

ss << "\t" << header_name.data() << ": " << header_value.data() << ",";

whatever follows the view, up to the next NUL, lands in the log. otlp_http_client.cc already writes seven other views with their length a few lines below, so these two were the outliers. Dropping .data() picks up nostd::operator<<, which is os.write(s.data(), s.length()).

examples/http/client.cc built a span attribute name the same way, with std::string(header_name.data()).

Reachability

The bundled curl client hands ForEachHeader views onto std::string, and std::string::data() is NUL terminated, so none of this misbehaves in tree today. HttpClientFactory is public API and Response::ForEachHeader takes string_view, so a client supplied through the factory can hand out views into its own receive buffer, which is where the log paths become reachable. The HttpSslOptions constructor is public and takes a string_view url directly.

Tests

HttpSslOptionsTests.UsesOnlyTheGivenUrlToDecideTheScheme covers the scheme decision, including the truncated view. It fails on the old code:

[  FAILED  ] HttpSslOptionsTests.UsesOnlyTheGivenUrlToDecideTheScheme

and passes on this one. It lives in url_parser_test, which links opentelemetry_ext without curl.

The two BuildResponseLogMessage methods are private members of file-local classes with no seam, so they have no direct test here. Their fix routes through nostd::operator<<, which writes the view's length. Say the word if you would rather I opened a seam for them.

Verification

Built with WITH_ELASTICSEARCH=ON WITH_OTLP_HTTP=ON WITH_ZIPKIN=ON WITH_EXAMPLES_HTTP=ON under OTELCPP_MAINTAINER_MODE=ON, clean. ./ci/do_ci.sh format reports no diff.

Not in this change

Properties::to_vector(span<string_view>) in the ETW exporter has the same std::string(item.data()) shape, and on the same lines it pre-sizes the result and then appends, so it returns twice as many entries as it was given with the first half empty. That is a second, unrelated defect in a component no CI job builds, so it is not folded in here.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.86%. Comparing base (1967428) to head (796d1a2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4346      +/-   ##
==========================================
- Coverage   80.86%   80.86%   -0.00%     
==========================================
  Files         450      450              
  Lines       19215    19216       +1     
==========================================
  Hits        15537    15537              
- Misses       3678     3679       +1     

Files with missing lines	Coverage Δ
...orters/elasticsearch/src/es_log_record_exporter.cc	12.22% <ø> (ø)
exporters/otlp/src/otlp_http_client.cc	70.76% <100.00%> (ø)
...nclude/opentelemetry/ext/http/client/http_client.h	85.72% <100.00%> (+1.85%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

Casting to a std::string seems wrong.

Did you intent to build a temporary std::string instead, as in:

span->SetAttribute("http.header." + std::string(header_name), header_value);

[thc1006]

You are right, and your form is the one I should have written. Changed in 59b31d0.

nostd::string_view carries an explicit operator std::string, which direct initialisation reaches just as the cast did, so the two compile to the same thing and yours says what it is doing. I checked it builds both ways the type can exist: the fallback nostd::string_view class, and WITH_STL=CXX17 where it becomes std::string_view and the standard library's own string_view constructor takes over instead. Clean with no warnings under maintainer mode in both.

The branch is also rebased onto main, since the CHANGELOG had picked up a conflict.

[marcalff]

LGTM, thanks for the fixes.