Bug: Memory leak of the last response headers after a nested HTTP request

[EdmondDantes]

php_stream_url_wrap_http() releases BG(last_http_headers) before the request and copies the new headers over it after the request. A request made from the notification callback of a running one stores its own headers there in between, and the outer copy overwrote that array without releasing it.

Reproduction (debug build, no extensions beyond pcntl and posix for the test server):

$ctx = stream_context_create([], ['notification' => function ($code) use (&$nested, $uri) {
    if ($code === STREAM_NOTIFY_MIME_TYPE_IS && !$nested) {
        $nested = true;
        file_get_contents($uri);
    }
}]);
file_get_contents($uri, false, $ctx);

Expected: no leak. Actual: === Total 4 memory leaks detected === (the nested request's header array).

The fix releases the stored array once, after the request, instead of before it. After the call http_get_last_response_headers() returns the outer request's headers, as before. During the request, a notification callback now sees the previous request's headers instead of null.

Test: ext/standard/tests/http/http_response_header_nested_request.phpt (fails on PHP-8.4 debug without the fix with the leak report, passes with it). The leak report comes from debug builds; a release build reports the leak only under ASAN or valgrind.

[ndossche]

Thanks.
This is a bit of a stupid edge case...
It also makes me wonder if the lines 1275-1276 should be moved to else branch of the check at line 1282; so we don't have to dtor the headers twice...

[EdmondDantes]

It also makes me wonder if the lines 1275-1276 should be moved to else branch of the check at line 1282; so we don't have to dtor the headers twice...

Like this?

php_stream *php_stream_url_wrap_http(
    php_stream_wrapper *wrapper,
    const char *path,
    const char *mode,
    int options,
    zend_string **opened_path,
    php_stream_context *context STREAMS_DC)
{
    php_stream *stream;
    zval headers;

    ZVAL_UNDEF(&headers);

    stream = php_stream_url_wrap_http_ex(
        wrapper, path, mode, options, opened_path, context,
        PHP_URL_REDIRECT_MAX, HTTP_WRAPPER_HEADER_INIT, &headers STREAMS_CC);

    if (!Z_ISUNDEF(headers)) {
        /* A request made from a notification callback may have stored its own headers meanwhile. */
        zval_ptr_dtor(&BG(last_http_headers));
        ZVAL_COPY(&BG(last_http_headers), &headers);

        if (FAILURE == zend_set_local_var_str(
                "http_response_header", sizeof("http_response_header")-1, &headers, 0)) {
            zval_ptr_dtor(&headers);
        }
    } else {
        zval_ptr_dtor(&BG(last_http_headers));
        ZVAL_UNDEF(&BG(last_http_headers));
    }

    return stream;
}

[ndossche]

Yes indeed