Skip to content

Commit 6bebe83

Browse files
committed
Fix use-after-free in stream freeing during shutdown
Streams will be freed in an unpredictable order during shutdown. Ignore explicit calls to php_stream_close() entirely to avoid use-after-free -- instead let the stream resource destructor deal with it. We have to account for a few special cases: * Enclosed streams should be freed, as the resource destructor will forward to the enclosing stream. * Stream cookies also directly free streams, because we delegate to the cookie destruction if one exists. * Mysqlnd also directly frees streams, because it explicitly removes stream resources (because mysqlnd!)
1 parent 2079b09 commit 6bebe83

File tree

3 files changed

+27
-19
lines changed

3 files changed

+27
-19
lines changed

ext/mysqlnd/mysqlnd_vio.c

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -644,18 +644,17 @@ MYSQLND_METHOD(mysqlnd_vio, close_stream)(MYSQLND_VIO * const net, MYSQLND_STATS
644644
if (net && (net_stream = net->data->m.get_stream(net))) {
645645
zend_bool pers = net->persistent;
646646
DBG_INF_FMT("Freeing stream. abstract=%p", net_stream->abstract);
647-
if (pers) {
648-
if (EG(active)) {
649-
php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE_PERSISTENT | PHP_STREAM_FREE_RSRC_DTOR);
650-
} else {
651-
/*
652-
otherwise we will crash because the EG(persistent_list) has been freed already,
653-
before the modules are shut down
654-
*/
655-
php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_RSRC_DTOR);
656-
}
647+
/* We removed the resource from the stream, so pass FREE_RSRC_DTOR now to force
648+
* destruction to occur during shutdown, because it won't happen through the resource. */
649+
/* TODO: The EG(active) check here is dead -- check IN_SHUTDOWN? */
650+
if (pers && EG(active)) {
651+
php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE_PERSISTENT | PHP_STREAM_FREE_RSRC_DTOR);
657652
} else {
658-
php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE);
653+
/*
654+
otherwise we will crash because the EG(persistent_list) has been freed already,
655+
before the modules are shut down
656+
*/
657+
php_stream_free(net_stream, PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_RSRC_DTOR);
659658
}
660659
net->data->m.set_stream(net, NULL);
661660
}

main/streams/cast.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ static int stream_cookie_closer(void *cookie)
8484

8585
/* prevent recursion */
8686
stream->fclose_stdiocast = PHP_STREAM_FCLOSE_NONE;
87-
return php_stream_free(stream, PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_KEEP_RSRC);
87+
return php_stream_free(stream,
88+
PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_KEEP_RSRC | PHP_STREAM_FREE_RSRC_DTOR);
8889
}
8990
#elif defined(HAVE_FOPENCOOKIE)
9091
static ssize_t stream_cookie_reader(void *cookie, char *buffer, size_t size)
@@ -126,7 +127,8 @@ static int stream_cookie_closer(void *cookie)
126127

127128
/* prevent recursion */
128129
stream->fclose_stdiocast = PHP_STREAM_FCLOSE_NONE;
129-
return php_stream_free(stream, PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_KEEP_RSRC);
130+
return php_stream_free(stream,
131+
PHP_STREAM_FREE_CLOSE | PHP_STREAM_FREE_KEEP_RSRC | PHP_STREAM_FREE_RSRC_DTOR);
130132
}
131133
#endif /* elif defined(HAVE_FOPENCOOKIE) */
132134

main/streams/streams.c

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -361,15 +361,22 @@ PHPAPI int _php_stream_free(php_stream *stream, int close_options) /* {{{ */
361361
int ret = 1;
362362
int preserve_handle = close_options & PHP_STREAM_FREE_PRESERVE_HANDLE ? 1 : 0;
363363
int release_cast = 1;
364-
php_stream_context *context = NULL;
364+
php_stream_context *context;
365365

366-
/* on an resource list destruction, the context, another resource, may have
367-
* already been freed (if it was created after the stream resource), so
368-
* don't reference it */
369-
if (EG(active)) {
370-
context = PHP_STREAM_CONTEXT(stream);
366+
/* During shutdown resources may be released before other resources still holding them.
367+
* When only resoruces are referenced this is not a problem, because they are refcounted
368+
* and will only be fully freed once the refcount drops to zero. However, if php_stream*
369+
* is held directly, we don't have this guarantee. To avoid use-after-free we ignore all
370+
* stream free operations in shutdown unless they come from the resource list destruction,
371+
* or by freeing an enclosed stream (in which case resource list destruction will not have
372+
* freed it). */
373+
if ((EG(flags) & EG_FLAGS_IN_SHUTDOWN) &&
374+
!(close_options & (PHP_STREAM_FREE_RSRC_DTOR|PHP_STREAM_FREE_IGNORE_ENCLOSING))) {
375+
return 1;
371376
}
372377

378+
context = PHP_STREAM_CONTEXT(stream);
379+
373380
if (stream->flags & PHP_STREAM_FLAG_NO_CLOSE) {
374381
preserve_handle = 1;
375382
}

0 commit comments

Comments
 (0)