Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

asiohiper.cpp: event_cb can be called with deallocated fdp pointer, causing crash #5090

Closed
andrewoliverhall opened this issue Mar 12, 2020 · 7 comments

Comments

@andrewoliverhall
Copy link

andrewoliverhall commented Mar 12, 2020

I did this

I am using the example code in asiohiper.cpp in a large application doing lots of http requests. We also use asio in other contexts. Often we run on osx using xcode "address sanitizer" and it flagged an issue in the asiohiper.cpp code, specifically in "event_cb". This issue was causing intermittent crashes in our application because of memory corruption.

The problem is that event_cb is passed as a function callback for async_read_some or async_write_some, where the boost::bind statement is capturing the pointer value of fdp as a parameter to event_cb. However, sometime between when async_read_some or async_write_some is invoked and when event_cb is called, the fdp pointer is deallocated via close_socket. This leads to an uninitialized memory read at the start of event_cb where it dereferences fdp.

My solution is to use a pattern I use elsewhere with asynchronous programming and boost::asio: weak_ptr. I created a struct such as:

struct SocketDesc : public std::enable_shared_from_this<SocketDesc>
{
        curl_socket_t                   mCurlSocket;
        boost::asio::ip::tcp::socket *  mBoostSocket;
        int*                            mFDP;
};

socket_map is changed to be a map from curl_socket_t to a std::shared_ptr.

Then I changed event_cb like this:

void Ntwk_CurlMulti::EventCB(const boost::system::error_code & error, std::size_t bytes, std::weak_ptr<SocketDesc> inWeakSocket, int action)
{
    auto sharedSocket = inWeakSocket.lock();
    if (!sharedSocket)
    {
        printf("%s event_cb: socket already closed", __FUNCTION__);
        return;
    }

    /* make sure the event matches what are wanted */
    if(*sharedSocket->mFDP == action || *sharedSocket->mFDP == CURL_POLL_INOUT) {
        CURLMcode rc;
        if(error)
            action = CURL_CSELECT_ERR;
        auto curlSocket = sharedSocket->mCurlSocket;
        sharedSocket.reset();
        rc = curl_multi_socket_action(mMulti, curlSocket, action, &mStillRunning);
        
        if (!CheckCurlError("event_cb: curl_multi_socket_action", rc))
            return;
        
        this->CheckMultiInfo();
        
        if(mStillRunning <= 0) {
            printf("%s last transfer done, kill timeout", __FUNCTION__);
            mTimer->cancel();
        }
        
        sharedSocket = inWeakSocket.lock();
        if (!sharedSocket)
        {
            printf("%s event_cb: socket closed after curl_multi_socket_action", __FUNCTION__);
            return;
        }
        
        /* keep on watching.
         * the socket may have been closed and/or fdp may have been changed
         * in curl_multi_socket_action(), so check them both */
        if(!error &&
           (*sharedSocket->mFDP == action || *sharedSocket->mFDP == CURL_POLL_INOUT) && mStillRunning) {
            boost::asio::ip::tcp::socket *tcp_socket = sharedSocket->mBoostSocket;
            if(action == CURL_POLL_IN)
                tcp_socket->async_read_some(boost::asio::null_buffers(), boost::bind(&Ntwk_CurlMulti::EventCB, this, _1, _2, inWeakSocket, CURL_POLL_IN));
            else if(action == CURL_POLL_OUT)
                tcp_socket->async_write_some(boost::asio::null_buffers(), boost::bind(&Ntwk_CurlMulti::EventCB, this, _1, _2, inWeakSocket, CURL_POLL_OUT));
        }
    }
}

There were various other changes to support this in the file. I like the weak_ptr technique but perhaps there is a more incremental way to solve the issue.

I expected the following

No crashes in my libcurl code.

curl/libcurl version

7.56.1. ( but this is a bug in the latest asiohiper.cpp on github, not a bug in libcurl exactly )

[curl -V output]

operating system

@bagder
Copy link
Member

bagder commented Mar 12, 2020

The asiohiper.cpp example is known to be buggy and it has a prominent warning at the top of the file:
WARNING: This example program is known to have flaws:

There seems to be very little interest or ability to fix it. I know I personally just get confused by asio so I'm not the man for the job.

Unfortunately, I would say that our best course of action right now - unless we get help to actually fix the example - is to simply remove the example source from the tarballs and web site.

@andrewoliverhall
Copy link
Author

andrewoliverhall commented Mar 12, 2020 via email

@bagder
Copy link
Member

bagder commented Apr 2, 2020

I will proceed and remove asiohiper.cpp before the next release unless we hear something else soon. I don't like how it causes problems for people and I think we''re better off without the example rather than shipping something that crashes occasionally when used.

@andrewoliverhall
Copy link
Author

andrewoliverhall commented Apr 3, 2020 via email

@MarcelRaad
Copy link
Member

This is a cpp file, so C++ is fine! ;-)

@andrewoliverhall
Copy link
Author

andrewoliverhall commented Apr 3, 2020 via email

bagder added a commit that referenced this issue May 2, 2020
This example has repeatedly been reported to contain bugs, and as users
copy and paste code from this into production, I now deem it better to
not provide the example at all.

Closes #5090
@bagder bagder closed this as completed in 9d47ff5 May 2, 2020
@jay
Copy link
Member

jay commented May 2, 2020

@andrewoliverhall though asiohiper has now been removed if the changes you made are still available please let us know, we could really use a working example of it. Thanks.

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

Successfully merging a pull request may close this issue.

4 participants