Skip to content

Fix stream_wrapper_unregister() resource leak #8587

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ PHP NEWS
- Streams:
. Set IP_BIND_ADDRESS_NO_PORT if available when connecting to remote host.
(Cristian Rodríguez)
. Fixed bug GH-8548 (stream_wrapper_unregister() leaks memory). (ilutov)

- Zip:
. add ZipArchive::clearError() method
Expand Down
40 changes: 40 additions & 0 deletions Zend/tests/gh8548.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
--TEST--
GH-8548: stream_wrapper_unregister() leaks memory
--FILE--
<?php

class Wrapper
{
public $context;

public function stream_open(string $path, string $mode, int $options): bool
{
return true;
}
}

function test() {
if (!stream_wrapper_register('foo', Wrapper::class)) {
throw new \Exception('Could not register stream wrapper');
}
if (!stream_wrapper_unregister('foo')) {
throw new \Exception('Could not unregister stream wrapper');
}
}

// The first iterations will allocate space for things like the resource list
for ($i = 0; $i < 5; $i++) {
test();
}

$before = memory_get_usage();
for ($i = 0; $i < 1000; $i++) {
test();
}
$after = memory_get_usage();

var_dump($before === $after);

?>
--EXPECT--
bool(true)
16 changes: 15 additions & 1 deletion main/streams/userspace.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,10 @@
static int le_protocols;

struct php_user_stream_wrapper {
php_stream_wrapper wrapper;
char * protoname;
zend_class_entry *ce;
php_stream_wrapper wrapper;
zend_resource *resource;
};

static php_stream *user_wrapper_opener(php_stream_wrapper *wrapper, const char *filename, const char *mode, int options, zend_string **opened_path, php_stream_context *context STREAMS_DC);
Expand Down Expand Up @@ -481,10 +482,12 @@ PHP_FUNCTION(stream_wrapper_register)
uwrap->wrapper.wops = &user_stream_wops;
uwrap->wrapper.abstract = uwrap;
uwrap->wrapper.is_url = ((flags & PHP_STREAM_IS_URL) != 0);
uwrap->resource = NULL;

rsrc = zend_register_resource(uwrap, le_protocols);

if (php_register_url_stream_wrapper_volatile(protocol, &uwrap->wrapper) == SUCCESS) {
uwrap->resource = rsrc;
RETURN_TRUE;
}

Expand All @@ -510,12 +513,23 @@ PHP_FUNCTION(stream_wrapper_unregister)
RETURN_THROWS();
}

php_stream_wrapper *wrapper = zend_hash_find_ptr(php_stream_get_url_stream_wrappers_hash(), protocol);
if (php_unregister_url_stream_wrapper_volatile(protocol) == FAILURE) {
/* We failed */
php_error_docref(NULL, E_WARNING, "Unable to unregister protocol %s://", ZSTR_VAL(protocol));
RETURN_FALSE;
}

ZEND_ASSERT(wrapper != NULL);
if (wrapper->wops == &user_stream_wops) {
struct php_user_stream_wrapper *uwrap = (struct php_user_stream_wrapper *)wrapper;
zend_resource *resource = uwrap->resource;
if (GC_DELREF(resource) == 0) {
// uwrap will be released by resource destructor
rc_dtor_func((zend_refcounted *)resource);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use something more standard like zend_list_delete here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't realize EG(regular_list) had a destructor attached to it that will free the resource. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpitnernalsbook.com has a good chapter about resources.


RETURN_TRUE;
}
/* }}} */
Expand Down