Skip to content

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

Merged
LamentXU123 merged 1 commit into
php:PHP-8.4from
LamentXU123:bug-fix-2
Jul 3, 2026
Merged

Fix GH-21705: ZipArchive::getFromIndex() ignores ZipArchive::FL_UNCHANGED for deleted entries#21706
LamentXU123 merged 1 commit into
php:PHP-8.4from
LamentXU123:bug-fix-2

Conversation

@LamentXU123

@LamentXU123 LamentXU123 commented Apr 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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

iliaal commented Apr 10, 2026

Copy link
Copy Markdown
Member

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.

@LamentXU123

LamentXU123 commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

@iliaal Could you please take a fast check on this PR? I want to get this fixed in 8.6. Thank you!

@iliaal

iliaal commented Jul 2, 2026

Copy link
Copy Markdown
Member

LGTM, just needs a NEWS entry

@LamentXU123

LamentXU123 commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Thank you!

@LamentXU123
LamentXU123 marked this pull request as draft July 3, 2026 05:56
@LamentXU123
Fix phpGH-21705: ZipArchive::getFromIndex() ignores FL_UNCHANGED (php…
d35ade5 
…#21706)

ZipArchive::getFromIndex() already passed flags to zip_fopen_index(), but it
used an unflagged stat call to determine the entry size. Use the same flags
for the stat lookup so deleted entries can still be read with
ZipArchive::FL_UNCHANGED, matching ZipArchive::getFromName().

Closes php#21706
@LamentXU123
LamentXU123 changed the base branch from master to PHP-8.4 July 3, 2026 06:12
@LamentXU123
LamentXU123 marked this pull request as ready for review July 3, 2026 06:12
@LamentXU123
LamentXU123 merged commit bc3a7c7 into php:PHP-8.4 Jul 3, 2026
1 of 2 checks passed
LamentXU123 added a commit that referenced this pull request Jul 3, 2026
@LamentXU123
Merge branch 'PHP-8.5'
041ef84 
* PHP-8.5:
  Fix GH-21705: ZipArchive::getFromIndex() ignores FL_UNCHANGED (#21706)
pull Bot pushed a commit to Ballalhossaintalukder/php-src that referenced this pull request Jul 3, 2026
@LamentXU123
Merge branch 'PHP-8.4' into PHP-8.5
c0f01b3 
* PHP-8.4:
  Fix phpGH-21705: ZipArchive::getFromIndex() ignores FL_UNCHANGED (php#21706)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Extension: zip

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

2 participants

@LamentXU123 @iliaal