[BUG] HttpOperation holds references to constructor arguments that its own default arguments do not outlive

[thc1006]

Describe your environment main at 3fb1d31, Ubuntu, gcc 14, libcurl 8.14.1, CMake, -fsanitize=address.

HttpOperation keeps a reference to four of its constructor arguments rather than a copy, in ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h:

  const opentelemetry::ext::http::client::HttpSslOptions &ssl_options_;   // :336
  const Headers &request_headers_;                                        // :338
  const opentelemetry::ext::http::client::Body &request_body_;            // :339
  const opentelemetry::ext::http::client::Compression &compression_;      // :344

Three of those four parameters have a default argument:

                const opentelemetry::ext::http::client::Headers &request_headers =
                    opentelemetry::ext::http::client::Headers(),
                const opentelemetry::ext::http::client::Body &request_body =
                    opentelemetry::ext::http::client::Body(),
                const opentelemetry::ext::http::client::Compression &compression =
                    opentelemetry::ext::http::client::Compression::kNone,

A default argument is a temporary created in the caller and destroyed at the end of the full-expression that contains the call, so a member reference bound to one dangles as soon as the constructor returns. The same happens for any caller that passes a prvalue for one of those parameters rather than a named object.

The header is installed, so this is reachable from outside the repository with nothing unusual: four arguments and the defaults do it.

Steps to reproduce

TEST_F(BasicCurlHttpTests, DefaultArgumentsDangle)
{
  http_client::HttpSslOptions no_ssl;
  curl::HttpOperation operation(http_client::Method::Get, "http://127.0.0.1:19000/get/", no_ssl,
                                nullptr);
  operation.Send();
}

Built with -fsanitize=address on main at 3fb1d31:

ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f9071000830
SUMMARY: AddressSanitizer: stack-use-after-scope
  ext/src/http/client/curl/http_operation_curl.cc:1182 in HttpOperation::Setup()
    #0 HttpOperation::Setup()  http_operation_curl.cc:1182
    #1 HttpOperation::Send()   http_operation_curl.cc:1402

:1182 is the read of compression_. The constructor itself is fine because it uses the parameters, which are alive for the length of the call. The dangling read is later, when Setup() reads the member.

What is the expected behavior? Constructing an HttpOperation and using it is defined, however the caller spells the arguments.

What is the actual behavior? The defaults, and any prvalue argument, leave the object holding references to storage that is already gone.

Additional context Nothing in this repository trips it. Session::SendRequest passes members of the Request it owns, and HttpClientSync::Get passes named locals, so all four references outlive the operation in both. The tests pass named locals too. So this is about what the installed header allows rather than about a failure anyone is seeing today.

Two ways out that I can see, and the choice is a design one rather than mine to make. Holding the four by value costs a copy of the headers and the body per request, which is the straightforward fix and matches what retry_policy_ already does. Deleting the three default arguments costs nothing at runtime and closes the shape that invites it, but leaves a prvalue argument dangling just the same. Happy to send either.

Found while writing a test that constructs an HttpOperation directly, on the way through the curl client work in #4391 and #4390.

[owent]

In my understanding , HttpOperation should noe exists without Session and Request. Copying these options may cause a lot, so it's unecessary to waste the CPU cost.

[thc1006]

In my understanding , HttpOperation should noe exists without Session and Request. Copying these options may cause a lot, so it's unecessary to waste the CPU cost.

Agreed on where this should end up. If the transfer state belongs to Session, there is nothing to hand over and the question stops existing.

I described my own patch badly above, so let me correct that. It does not copy on the path that matters. The constructor parameters become by value, but Session::SendRequest passes std::move for the SSL options, the headers and the body, so the in-tree call site hands its request over and copies nothing. By value is what lets it do that without changing a single existing call site. A caller passing an lvalue still copies, and every caller that does so today is a test.

That does not change the conclusion. The ownership belongs in Session, so I am holding the patch and taking the question to #4448.

The one thing I would rather not lose while that is decided is that this report is measured, not reasoned. ThreadSanitizer reports the write in Request::SetBody against the read of the same vector in the read callback, on every run of a case that posts a body and then writes to the request while the transfer is in flight. Separately, three of the members have default arguments, so an operation built without headers, a body or a compression setting binds them to temporaries that die at the end of the constructor call. If the redesign is going to take a while, a narrow interim fix is close to free here. If it is near, it is not worth the churn. I am happy either way, and I would rather you decide that than me.

One correction to my report: after #4327, http_client_curl.h and http_operation_curl.h are no longer installed by the CMake package. Bazel //ext still exposes the whole include tree, so direct Bazel and in-tree construction remain affected, but the current CMake-installed surface is narrower than I wrote.