Skip to content

Fix GH-21705: ZipArchive::getFromIndex() ignores ZipArchive::FL_UNCHANGED for deleted entries#21706

Open
LamentXU123 wants to merge 4 commits intophp:masterfrom
LamentXU123:bug-fix-2
Open

Fix GH-21705: ZipArchive::getFromIndex() ignores ZipArchive::FL_UNCHANGED for deleted entries#21706
LamentXU123 wants to merge 4 commits intophp:masterfrom
LamentXU123:bug-fix-2

Conversation

@LamentXU123
Copy link
Copy Markdown
Contributor

@LamentXU123 LamentXU123 commented Apr 10, 2026

fixes #21705 by passing the correct flag into PHP_ZIP_STAT_INDEX

The code is as follows:

	if (type == 1) {
		if (zend_parse_parameters(ZEND_NUM_ARGS(), "P|ll", &filename, &len, &flags) == FAILURE) {
			RETURN_THROWS();
		}

		ZIP_FROM_OBJECT(intern, self);

		PHP_ZIP_STAT_PATH(intern, ZSTR_VAL(filename), ZSTR_LEN(filename), flags, sb);
	} else {
		if (zend_parse_parameters(ZEND_NUM_ARGS(), "l|ll", &index, &len, &flags) == FAILURE) {
			RETURN_THROWS();
		}

		ZIP_FROM_OBJECT(intern, self);

		PHP_ZIP_STAT_INDEX(intern, index, 0, sb); // bug is here
	}

when mode == 1 its for getFromName, where it calls PHP_ZIP_STAT_PATH with the correct arguments flags

PHP_ZIP_STAT_PATH(intern, ZSTR_VAL(filename), ZSTR_LEN(filename), flags, sb);

when mode == 0 its for getFromIndex, where it calls PHP_ZIP_STAT_INDEX with flag be forced to 0

PHP_ZIP_STAT_INDEX(intern, index, 0, sb);

It's quite bizarre.

@LamentXU123 LamentXU123 marked this pull request as draft April 10, 2026 10:05
@LamentXU123 LamentXU123 marked this pull request as ready for review April 10, 2026 10:07
@iliaal
Copy link
Copy Markdown
Contributor

iliaal commented Apr 10, 2026

One pre-existing concern worth flagging while we're touching this line: the flags argument is zend_long on our side and zip_flags_t (uint32_t) on libzip's side, and nothing validates the range on the way across.

I ran this on master + this patch:

$zip->getFromName('bar.txt', 0, -1);          // receives 0xFFFFFFFF (all libzip flags set)
$zip->getFromName('foo.txt', 0, PHP_INT_MAX); // receives lower 32 bits silently
$zip->getFromName('bar.txt', 0, 0x100000000); // truncates to 0, FL_UNCHANGED intent lost silently

The third case is the worst: the caller's flag intent lives in the upper 32 bits, truncates to zero on the way into libzip, and they get bool(false) back with no error to tell them why. libzip doesn't crash on unknown bits today (it masks them), so this is silent wrong behavior rather than UB, but it's fragile against future libzip flag additions.

The same pattern shows up on all 15 zend_long flags declarations in ext/zip/php_zip.c. This PR doesn't make it worse, it just inherits the situation. Probably worth a separate issue for a shared helper that validates 0 <= flags <= UINT32_MAX (and optionally rejects bits outside the known ZIP_FL_* mask) before handing off to libzip.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ZipArchive::getFromIndex() ignores ZipArchive::FL_UNCHANGED for deleted entries

2 participants