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

[andrewoliverhall]

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]

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]

Hey Daniel - Thanks for the quick response. So I have experience with asio because we use it in various parts of our app, and I actually really like it - but maybe I am the minority. Anyway, our modified asiohiper.cpp code - not just the snippet I put in the bug but the rest of the supporting changes - solves the issue and is working out great for us. Let me check with my company about just submitting that whole thing. I like the model of being able to use asio and have the number of threads be tunable independently of the number of sockets. That is really nice.

…

-Andy

On Thu, Mar 12, 2020 at 6:21 AM Daniel Stenberg ***@***.***> wrote: The asiohiper.cpp example is known to be buggy and it has 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 exampled from the tarballs and web site. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5090 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOZ2DCDL3K4IA3JB4IJDULTRHDOVTANCNFSM4LGMBE2Q> .

[bagder]

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]

Hey Daniel - Yeah this slipped my mind for a while, we have been the in the throws of a big release at my company which wraps up soon. So our fix is looking good - our asio based curl code has been stable for months after I made the fix. If I were able to apply the fix to the sample code but make it use c++ features, would that be useful or are you trying to keep everything in straight c?

…

-Andy

On Thu, Apr 2, 2020 at 3:24 PM Daniel Stenberg ***@***.***> wrote: 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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5090 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOZ2DCBKTYPLIIPZDNRLFRLRKUGCPANCNFSM4LGMBE2Q> .

[MarcelRaad]

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

[andrewoliverhall]

Oops, sorry not enough coffee yet I was mixing it up with other files.

…

On Fri, Apr 3, 2020 at 7:45 AM Marcel Raad ***@***.***> wrote: This is a cpp file, so C++ is fine! ;-) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5090 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AOZ2DCEOC276MZ2G7UC4JR3RKXZCNANCNFSM4LGMBE2Q> .

[jay]

@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.