Skip to content

zip: require remove_path to be shorter than the path it strips - #22973

Merged
iliaal merged 1 commit into
php:masterfrom
iliaal:fix/zip-remove-path-exact-match
Jul 31, 2026
Merged

zip: require remove_path to be shorter than the path it strips#22973
iliaal merged 1 commit into
php:masterfrom
iliaal:fix/zip-remove-path-exact-match

Conversation

@iliaal

@iliaal iliaal commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

zend_string_starts_with() also matches on equality, so the length check dropped in bcb05dc was load-bearing. A globbed path that equals remove_path now reads its own NUL terminator in the IS_SLASH test, falls into the non-slash branch and strips its whole length, so the entry lands in the archive under an empty name. Before the conversion the path was kept as-is, which is what 8.4 and 8.5 still do.

$zip->addGlob("$dir/*.txt", 0, ['remove_path' => "$dir/a.txt"]);
// a.txt is added as "", b.txt keeps its full path

The zend_string conversion in bcb05dc replaced the length check with
zend_string_starts_with(), which also matches on equality. A path equal to
remove_path then hits the non-slash branch, strips its whole length and
produces an empty archive entry name instead of keeping the path.

Closes phpGH-22973

@LamentXU123 LamentXU123 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thank you!

@iliaal
iliaal merged commit a184a51 into php:master Jul 31, 2026
18 checks passed
@LamentXU123

Copy link
Copy Markdown
Member

This is indeed a behavior change comparing to alpha3. Should a NEWS entry be added?

@iliaal

iliaal commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Not sure if a change in alpha code qualifies, since behavior is not really "stable".

@LamentXU123

LamentXU123 commented Jul 31, 2026

Copy link
Copy Markdown
Member

Since there indeed are examples that qualifies (#22907) Let's ask Matteo for opinions as a 8.6 RM which deal with this before :) @mbeccati

@mbeccati

Copy link
Copy Markdown
Contributor

If I understand correctly, this is a bug fix for bcb05dc, which didn't land in alpha3. The fix restores the old behaviour (8.4+).

If my recap is correct, no NEWS entry is required.

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.

3 participants