[mlederhi]

in the case where size is between page_size and 2 * page_size.

[artyom-beilis]

Can you give a case it fails? How it fixes the issue? I tried setting small page sizes and it all works.

Also I don't see the bug here. The idea is following if it is a long string allocate a separate page for it, otherwise use paged pool.

[artyom-beilis]

I also rerun some cases with small buffer size with valgrind to see if anythonig shows up and it does not.

Can you please explain the fix and what it fixes.

[artyom-beilis]

The reason for size * 2 - is to prevent wasting too much memory for large strings.

[mlederhi]

The problem is the case were the size is between page size and 2 * page size.

It's too small to allocate an own page with the correct size ( > 2 * page size case).

Then depending on the size it allocates a new page or uses an existing page. But since the size is > page size it always writes the string out of bounds because a page has a maximum size of page_size and that is smaller than size.

[mlederhi]

Note that I don't know why it worked with CppCMS 1.2.1 (same code in string_map.h). But with CppCMS 2.0.1 we started experiencing problems in only one of our applications. I found the origin of the problem with Valgrind's memcheck.

[DanielJung]

We also recently had a bug in our code that seems to be related to this.
At the very bottom is the curl command that caused a crash for us and was reproducible in our application.
I tried to recreate a short dummy program using cppcms's own tools:

The allocation sequence tries to simulate what the HTTP connector does per request:
src/http_api.cpp reset_all() runs pool_.clear(); pool_.add(ip); pool_.alloc(10);, then parse_single_header() does pool_.alloc(name+1) and
pool_.alloc(value+1) for every header. pool_ is default-constructed
(page_size_ = 2048).

// g++ -std=c++11 -g -fsanitize=address repro_lib.cpp -o repro_lib \
//     -I<cppcms-src>/private -I<cppcms-install>/include
#include <string>
#include <vector>
#include "string_map.h"            // the real cppcms header, unmodified

using cppcms::impl::string_pool;
typedef std::vector<std::pair<std::string, std::string> > headers_t;

static void serve_request(string_pool &pool, const headers_t &headers)
{
    pool.clear();
    pool.add("127.0.0.1");
    pool.alloc(10);
    for(size_t i = 0; i < headers.size(); i++) {
        pool.alloc(headers[i].first.size() + 1);
        pool.alloc(headers[i].second.size() + 1);
    }
}

int main()
{
    string_pool pool;

    headers_t req;
    req.push_back(std::make_pair("host", "localhost"));
    req.push_back(std::make_pair("connection", "keep-alive"));
    req.push_back(std::make_pair("cookie", "sid=" + std::string(1200, 'A'))); // >1024 -> own page
    for(int i = 0; i < 15; i++)                                               // ordinary headers
        req.push_back(std::make_pair("x-header", std::string(120, 'v')));

    serve_request(pool, req);     // request N
    serve_request(pool, req);     // request N+1 on same keep-alive connection -> overflow
    return 0;
}

AddressSanitizer:

ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 121 ... 0 bytes after 1221-byte region
    #0 memset
    #1 cppcms::impl::string_pool::alloc(unsigned long)            string_map.h:51
0x... is located 0 bytes after 1221-byte region
allocated by thread T0 here:
    #1 cppcms::impl::string_pool::allocate_space(unsigned long)   string_map.h:106
    #2 cppcms::impl::string_pool::alloc(unsigned long)            string_map.h:50

It triggers whenever a request stores a single pooled string larger than
page_size_ / 2 (~1 KB) — a long header value such as Cookie, or a long
request URI / query string, both of which go through this pool — and another
request follows on the same keep-alive connection. The overflow happens on that
next request, so it surfaces as free(): invalid next size or a corrupted-heap
crash. A single request that closes the connection does not show it: the pool
starts fresh each connection.

Reproducing against a running server (curl)

Point a server at api: http and reach it directly (no proxy that forces
Connection: close upstream). curl reuses one keep-alive connection for two
URLs. The first URL has a ~1.1 KB path, which the connector stores as one pooled
string (request_uri_ = pool_.add(...)), over the page_size_ / 2 threshold.
The five padding headers (each value ~300 bytes, below the threshold, so
bump-allocated) apply to both requests and, on the second request, run past the
oversized page kept from the first:

P=$(printf 'a%.0s' {1..1100})
H=$(printf 'b%.0s' {1..300})
curl -s -o /dev/null -H 'Connection: keep-alive' \
  -H "X-Pad-1: $H" -H "X-Pad-2: $H" -H "X-Pad-3: $H" -H "X-Pad-4: $H" -H "X-Pad-5: $H" \
  "http://HOST:PORT/$P" \
  "http://HOST:PORT/"

The worker crashes on the second request. A single URL (one request, connection
closed) does not crash it.

[artyom-beilis]

The worker crashes on the second request. A single URL (one request, connection

Wow reproduced! serious bug indeed.

Thanks!

I'm going to work on it ASAP!

[artyom-beilis]

I feel dump on this code:

abc908e

It was incorrect pool cleanup - totally messed up! How could I write this broken code?!

I fixed it in master branch now it works and does not crash

Thanks for the way to reproduce it! It was of enromous help.

I'll live PL for few days opened for discussion of this horrible bug. I'll close it later (since the fix is different)

[artyom-beilis]

And added ,memory leak fix

9b03c1d