Skip to content

Conversation

@NattyNarwhal
Copy link
Member

The header removal code looked for the colon for key-value at the wrong place, so it would overzealously remove headers. Tweak that condition, and make the alternative condition only active if it's set (with the remove prefix op case).

Fixes GH-21018.

The header removal code looked for the colon for key-value at the wrong
place, so it would overzealously remove headers. Tweak that condition,
and make the alternative condition only active if it's set (with the
remove prefix op case).

Fixes phpGH-21018.
@NattyNarwhal NattyNarwhal requested a review from bukka as a code owner January 23, 2026 20:13
@NattyNarwhal NattyNarwhal changed the title Fix regression with header removing removing whole prefixes Fix regression with header removal removing whole prefixes Jan 23, 2026
Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks correct to me otherwise, as a person who doesn't know this code well.

main/SAPI.c Outdated
* since zend_llist_del_element only removes one matched item once,
* we should remove them manually
*/
static void sapi_remove_header(zend_llist *l, char *name, size_t len, size_t header_len)
Copy link
Member

Choose a reason for hiding this comment

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

I find the name header_len very confusing. Maybe rename to prefix_len? It doesn't just include the header name fwiu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, this is pretty confusing. Not sure what I was thinking then...

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Great, thanks!

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.

2 participants