Fix GH-17139: Fix zip_entry_name() crash on invalid entry. #17142
Conversation
Increasing the GC refcount when reading the zip entry before zip_entry_name() fetches the info, leading to a dangling pointer otherwise.
| @@ -1257,6 +1257,7 @@ PHP_FUNCTION(zip_read) | |||
|
|
|||
| zr_rsrc->zf = zip_fopen_index(rsrc_int->za, rsrc_int->index_current, 0); | |||
| if (zr_rsrc->zf) { | |||
| Z_ADDREF_P(zip_dp); | |||
There was a problem hiding this comment.
This might lead to a memory leak. Wouldn't the refcount increase for every zip_read()?
There was a problem hiding this comment.
ASAN nor DEBUG build reported anything on this new test nor in zip_read.phpt
There was a problem hiding this comment.
Argh – resources! For a clean solution, the le_zip_entry resources would need to store a reference to their le_zip_dir resource; otherwise, as happens here, if the le_zip_dir resource is closed, there are dangling pointers. Increasing the refcount here (and possibly elsewhere) only shades the problem. Given that the procedural API is deprecated as of PHP 8.0.0, I'd opt for wontfix. @remicollet, what do you think?
There was a problem hiding this comment.
If that s obsolete for that long, might be best remove it for 8.5/9.0 or so many people still using this api ?
There was a problem hiding this comment.
I think the plan is to remove for PHP 9 (removing in a minor is quite a BC break). And I guess most are using the OO-API already.
|
Yeah this patch will leak memory until the request end. The reason you don't see the leak in ASAN/Valgrind is because that memory is cleaned up anyway when the regular list is destroyed. So the practical effect is that this will delay destruction (and writes ig) of the zip and that for long running processes we will leak memory for a long time. Why don't we just do this instead? Seems simple enough. diff --git a/ext/zip/php_zip.c b/ext/zip/php_zip.c
index 0c1dfaf5dd1..ca48a0c993f 100644
--- a/ext/zip/php_zip.c
+++ b/ext/zip/php_zip.c
@@ -1255,6 +1255,7 @@ PHP_FUNCTION(zip_read)
RETURN_FALSE;
}
+ zr_rsrc->zip_rsrc_handle = Z_RES_P(zip_dp)->handle;
zr_rsrc->zf = zip_fopen_index(rsrc_int->za, rsrc_int->index_current, 0);
if (zr_rsrc->zf) {
rsrc_int->index_current++;
@@ -1371,7 +1372,7 @@ static void php_zip_entry_get_info(INTERNAL_FUNCTION_PARAMETERS, int opt) /* {{{
RETURN_THROWS();
}
- if (!zr_rsrc->zf) {
+ if (!zr_rsrc->zf || !zend_hash_index_exists(&EG(regular_list), zr_rsrc->zip_rsrc_handle)) {
RETURN_FALSE;
}
diff --git a/ext/zip/php_zip.h b/ext/zip/php_zip.h
index 41392d1967d..13ea7fdc001 100644
--- a/ext/zip/php_zip.h
+++ b/ext/zip/php_zip.h
@@ -60,6 +60,7 @@ typedef zip_rsrc * zip_rsrc_ptr;
typedef struct _ze_zip_read_rsrc {
struct zip_file *zf;
struct zip_stat sb;
+ zend_long zip_rsrc_handle;
} zip_read_rsrc;
/* Extends zend object */
|
|
Oh, looks like we have a private header for once. :) |
Don't increment the refcount, but latter remember the ID to check afterwards whether the resource still exists. Replaces phpGH-17142.
Don't increment the refcount, but latter remember the ID to check afterwards whether the resource still exists. Replaces phpGH-17142.
|
I filed my fix as PR: #17435 |
Don't increment the refcount, but latter remember the ID to check afterwards whether the resource still exists. Replaces phpGH-17142.
Increasing the GC refcount when reading the zip entry before zip_entry_name() fetches the info, leading to a dangling pointer otherwise.