Memory corruption in http::client::post #482

Closed
urbanyoung opened this Issue Jan 1, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@urbanyoung

I'm using the latest stable release (0.11.1-final) downloaded from http://cpp-netlib.org/ and have noticed that references to a local object are being stored for certain post requests. I apologize in advance for the poor quality of this report, but the code I downloaded from your website doesn't seem to match the repo. For instance, in the downloaded code the problem file is boost/network/protocol/http/client/facade.hpp and the closest I can find in the repo is https://github.com/cpp-netlib/cpp-netlib/blob/master/http/src/network/protocol/http/client/facade.ipp#L46

Regardless, they both have the same issue.

The linked function operates on a local copy of the request object which then gets passed, by reference, to request_skeleton. Eventually host() is called on this object, and the result is stored. Later on, when the newly created thread tries to access the stored data, it is no longer valid because the local request object has long since been destroyed.

I don't have a proposed fixed because I'm not familiar enough with the library, but in my own code I simply changed post to accept a reference, and ensure the request remains valid for the duration of the post.

Hope that makes sense.

@deanberris deanberris added the BUG label Jan 2, 2015

@deanberris deanberris added this to the 0.11.2 milestone Jan 2, 2015

@deanberris deanberris self-assigned this Jan 2, 2015

@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Jan 2, 2015

Member

You're looking at the wrong version of the file -- the link you have is to the master branch, which is not where 0.11-devel comes from.

That said, I think there's a potential for this to actually be a problem. I'm going to see if I can actually fix this in 0.11.2 (or whether someone else can).

Member

deanberris commented Jan 2, 2015

You're looking at the wrong version of the file -- the link you have is to the master branch, which is not where 0.11-devel comes from.

That said, I think there's a potential for this to actually be a problem. I'm going to see if I can actually fix this in 0.11.2 (or whether someone else can).

@joshuanapoli

This comment has been minimized.

Show comment
Hide comment
@joshuanapoli

joshuanapoli Mar 11, 2015

The specific problem is that basic_client_facade::post takes the request argument by value. A reference to the new request is passed through:

  1. async_client<...>::request_skeleton.
  2. async_connection_policy<...>::connection_impl::send_request
  3. http_async_connection<...>::start

In http_async_connection<...>::start, the result of host(request) is bound to a function and attached to a strand. The result type of host(request) is a host_wrapper. This has reference semantics.

Unfortunately, the referenced request goes out of scope when basic_client_facade::post returns. This leaves a dangling reference. The dangling reference eventually leads to undefined behavior when it is used in the processing of callbacks for the connection and message exchange.

The put request has the same flaw.

Also, the documentation/tutorial should probably make it more clear that the application is responsible for ensuring the lifetime of these values lasts until the request has terminated. Managing this lifetime takes some care when using the client interface in an asynchronous pattern.

The specific problem is that basic_client_facade::post takes the request argument by value. A reference to the new request is passed through:

  1. async_client<...>::request_skeleton.
  2. async_connection_policy<...>::connection_impl::send_request
  3. http_async_connection<...>::start

In http_async_connection<...>::start, the result of host(request) is bound to a function and attached to a strand. The result type of host(request) is a host_wrapper. This has reference semantics.

Unfortunately, the referenced request goes out of scope when basic_client_facade::post returns. This leaves a dangling reference. The dangling reference eventually leads to undefined behavior when it is used in the processing of callbacks for the connection and message exchange.

The put request has the same flaw.

Also, the documentation/tutorial should probably make it more clear that the application is responsible for ensuring the lifetime of these values lasts until the request has terminated. Managing this lifetime takes some care when using the client interface in an asynchronous pattern.

joshuanapoli added a commit to joshuanapoli/cpp-netlib that referenced this issue Mar 11, 2015

@deanberris

This comment has been minimized.

Show comment
Hide comment
@deanberris

deanberris Mar 11, 2015

Member

@joshuanapoli Wow -- that's a great analysis! Do you think you can fix this?

Member

deanberris commented Mar 11, 2015

@joshuanapoli Wow -- that's a great analysis! Do you think you can fix this?

@joshuanapoli

This comment has been minimized.

Show comment
Hide comment
@joshuanapoli

joshuanapoli Mar 11, 2015

I think that the fix is to simply convert the host_wrapper to a string before binding. #506 fixes the 0.11 branch. I had a reproducible crash in my application that is fixed by this change.

I think that the fix is to simply convert the host_wrapper to a string before binding. #506 fixes the 0.11 branch. I had a reproducible crash in my application that is fixed by this change.

@deanberris deanberris closed this Mar 11, 2015

@brad-anderson

This comment has been minimized.

Show comment
Hide comment
@brad-anderson

brad-anderson Aug 19, 2015

I'm surprised a new release hasn't been made for this issue. Not being able to post or put without crashing in an http library seems like the type of thing that would warrant a hotfix.

I switched to the 0.11-devel branch and it cleared up for me.

I'm surprised a new release hasn't been made for this issue. Not being able to post or put without crashing in an http library seems like the type of thing that would warrant a hotfix.

I switched to the 0.11-devel branch and it cleared up for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment