Skip to content

Fix #76954 Use while loop in func add_response_header #3566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

stodorovic
Copy link
Contributor

Related issues:

https://bugs.php.net/bug.php?id=76954
https://github.com/Automattic/wp-super-cache/issues/539

PHP 5.6 uses while loop, but PHP 7.x uses do ... while which decreases len before condition. Parameter len in function memcpy has decreased length and last character isn't copied.

@staabm
Copy link
Contributor

staabm commented Oct 1, 2018

Could you add a unit test?

@cmb69
Copy link
Member

cmb69 commented Oct 1, 2018

Indeed, the refactoring in 4cba2f9 looks fishy. If len == 1 we would access h->header[-1]. It seems to me that we could stick with the do-loop, if we change the indexes from len-1 to len in the while-clause. @weltling Am I missing something?

Regarding the unit test: I'm not sure if we can use the --HEADERS-- section since the docs say it would only be supported by server-tests.php, but sapi/tests/test006.phpt and sapi/tests/test007.phpt use it.

@stodorovic
Copy link
Contributor Author

Regarding to do ... while, possible solution is:

if (NULL != p) {
	len = p - h->header + 1;
}
if (len > 0) {
	do {
		len--;
	} while (len != 0 && (h->header[len-1] == ' ' || h->header[len-1] == '\t'));

It works fine even if : is first or last character (which doesn't make sense).

@cmb69 If we use h->header[len] in condition, then correct solution is:

if (NULL != p) {
	len = p - h->header;
}
if (len > 0) {
	do {
		len--;
	} while (len != 0 && (h->header[len] == ' ' || h->header[len] == '\t'));
	if (len) {
		len++; // Include last character.

If len == 0 then condition is false without evaluation of h->header[len-1] (regardless to type of loop). So, I think that's better to use while loop because we should keep original value of len (if whitespace doesn't exist) and it's little faster (and more clear solution for me).

I've added test and we don't need --HEADERS-- because we should use header function to send header.

@cmb69
Copy link
Member

cmb69 commented Oct 1, 2018

@stodorovic Thanks, that sounds plausible. (And yes, I've confused apache_response_headers with apache_request_headers).


passthru("$php -n -q $filename");

@unlink($filename);
Copy link
Member

Choose a reason for hiding this comment

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

This should be in a --CLEAN-- section for consistency


file_put_contents($filename, $code);

passthru("$php -n -q $filename");
Copy link
Member

@KalleZ KalleZ Oct 1, 2018

Choose a reason for hiding this comment

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

Since this is a full path (from the $filename variable), the $filename should be quoted for escapes in case the full path contains a space

@php-pulls
Copy link

Comment on behalf of cmb at php.net:

Thanks! Applied via 47b89bc.

@php-pulls php-pulls closed this Oct 8, 2018
@stodorovic stodorovic deleted the cgi_add_response_header_bug_76954 branch October 20, 2018 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants