Remove handle when pipelining enabled corrupts data

[molind]

I did this

Our app downloads only visible tiles and cancels tile download task when some tile is out of view. Recently we changed app to use piping connections. With some chance curl corrupted tiles. Seems curl_multi called write function for wrong curl_easy with data from cancelled curls.

I expected the following

No data should be sent to write functions after curl_easy_cleanup call.

curl/libcurl version

curl 7.56.1

operating system

iOS 11.1
macOS 10.12

Our minimal test example below. Put breakpoint on line 59.

//
//  main.cpp
//  TestMulti
//
//  Created by Arkadiy Tolkun on 11/17/17.
//  Copyright © 2017 Arkadiy Tolkun. All rights reserved.
//

#include <map>
#include <string>
#include <curl/curl.h>
#include <stdio.h>

const int MaxTasks = 4;
const int DownloadTimeout = 30;
const int LowSpeedTimeout = 15;
const int LowSpeedLimit = 10; // bytes per second

struct NetBuffer
{
    size_t _pos;
    size_t _size;
    void *_buf;
    
    NetBuffer() :
    _pos(0),
    _size(0),
    _buf(nullptr)
    {
    }
    
    ~NetBuffer()
    {
        if(_buf)
            free(_buf);
    }
};

size_t wdata(const char *ptr, size_t size, size_t nmemb, NetBuffer *pthis)
{
    printf("write_data(%p, %ld, %ld, %p)\n", ptr, size, nmemb, pthis);
    
    uint32_t nextSize = (uint32_t)(size*nmemb);
    if(pthis->_pos + nextSize > pthis->_size)
    {
        uint32_t allocSize = nextSize;
        if (allocSize < 65536)
            allocSize = 65536;
        
        pthis->_size += allocSize;
        pthis->_buf = realloc(pthis->_buf, pthis->_size);
    }
    if(!pthis->_buf)
        return 0;
    
    // PNG magic number shouldn't be found in the middle of the file
    const unsigned char magic[] = {0x89, 0x50, 0x4e, 0x47};
    if (pthis->_pos != 0 && memcmp(magic, ptr, sizeof(magic)) == 0)
        printf("wtf");
    
    memcpy((char *)pthis->_buf + pthis->_pos, ptr, nextSize);
    pthis->_pos += nextSize;
    return size * nmemb;
}

int main(int argc, const char * argv[])
{
    std::string testUrls[] = {
        "http://a.tile.openstreetmap.org/5/%d/%d.png",
        "http://b.tile.openstreetmap.org/5/%d/%d.png",
        "http://c.tile.openstreetmap.org/5/%d/%d.png",
    };
    
    auto multi = curl_multi_init();
    curl_multi_setopt(multi, CURLMOPT_PIPELINING, CURLPIPE_HTTP1 | CURLPIPE_MULTIPLEX);
    int runing = 0;
    
    std::map<CURL *, NetBuffer *> map;
    while (1)
    {
        int tmp;
        while (curl_multi_perform(multi, &tmp) == CURLM_CALL_MULTI_PERFORM)
        {
        }
        
        CURLMsg *msg=nullptr;
        int msgs_left=0;
        while ((msg = curl_multi_info_read(multi, &msgs_left)))
        {
            if (msg->msg == CURLMSG_DONE)
            {
                CURL *curl = msg->easy_handle;
                curl_multi_remove_handle(multi, curl);
                auto it = map.find(curl);
                if(it!=map.end())
                {
                    delete it->second;
                    map.erase(it);
                }
                curl_easy_cleanup(curl);
                runing--;

                
                if (rand() % 100 < 50 && map.size()) 
                {
                    auto task = map.begin();
                    
                    curl_multi_remove_handle(multi, task->first);
                    delete task->second;
                    curl_easy_cleanup(task->first);
                    
                    map.erase(task);
                    runing--;
                }
            }
        }
        
        while(runing<MaxTasks)
        {
            CURL *curl = curl_easy_init();
            if(curl)
            {
                NetBuffer *buf = new NetBuffer();
                map[curl] = buf;
                
                char url[256];
                
                snprintf(url, sizeof(url), testUrls[rand() % 3].c_str(), rand() % 32, rand() % 32);
                
                curl_easy_setopt(curl, CURLOPT_URL, url);
                
                curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, "gzip, deflate");
                curl_easy_setopt(curl, CURLOPT_USERAGENT, "Agent");
                
                curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
                curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
                curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, DownloadTimeout);
                curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, LowSpeedTimeout);
                curl_easy_setopt(curl, CURLOPT_LOW_SPEED_LIMIT, LowSpeedLimit);
                curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, wdata);
                curl_easy_setopt(curl, CURLOPT_WRITEDATA, buf);
                
                curl_easy_setopt(curl, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);
                curl_easy_setopt(curl, CURLOPT_PIPEWAIT, 1L);
                
                //curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
                curl_multi_add_handle(multi, curl);
            }
            runing++;
        }
        int numfds=0;
        curl_multi_wait(multi, NULL, 0, 250, &numfds);
    }
    return 0;
}

[bagder]

Possibly related to this known bug: Disabling HTTP Pipelining

[molind]

@bagder As you may see we're not cancel Pipelining. We just remove curl_easy handle from curl_multi.

                    curl_multi_remove_handle(multi, task->first);
                    delete task->second;
                    curl_easy_cleanup(task->first);

[bagder]

I haven't investigated the issue closely, but I would still presume the problems to be very similar. When a handle is used for pipelining it needs to be removed (or cancelled) with care. I'm not even entirely sure what we should consider the correct behavior to be if that handle is in fact used in the pipeline. I suppose the entire connection needs to be killed and the other transfers involved in the pipeline has to be cancelled.

[molind]

@bagder Now i see your point. Since using pipelining we can't say to the other side "stop sending us this file", the only way to stop transmission of cancelled data is to close connection.

[bagder]

Right, since when you remove handle A that is associated with a pipeline, handle B's request may already have been sent and is queued for a response on the same connection after A's response so when you remove A (before its entire response has been received), you need to disconnect the connection since there won't be anyone around anymore to read the A response, but you also then kill the connection for handle B, whose response hasn't even arrived yet.

[molind]

Ok, seems we have to remove canceling from our downloading queue, for tasks that are already added into curl_multi.

Could I suggest to add some warning printed in verbose mode? Like "hey, when you remove curl_easy from pipelined curl_multi you'll probably screw your other curl_easy handlers".

[bagder]

I suppose an ideal solution would be to do the removal but to save enough for the moment to let handle B get its response and then once handle A is completely off the hook remove the rest.

Could I suggest to add some warning

I would prefer an attempt to a real fix...

[bagder]

Now mentioned in the KNOWN_BUGS's 1.2 section. Closing until someone works on this.