Skip to content

Conversation

MegaManSec
Copy link

No description provided.

@smalyshev
Copy link
Contributor

Any tests/reproductions for these fixes?

@MegaManSec
Copy link
Author

No.

I assume if you run the command 100 times and compare the results, you might get different data.
But in reality, it is hard to trigger. It's really just "undefined behavior", which in the past has led to exploits.

Thanks,

@@ -2255,6 +2255,7 @@ int php_module_startup(sapi_module_struct *sf, zend_module_entry *additional_mod

zuv.html_errors = 1;
zuv.import_use_extension = ".php";
zuv.import_use_extension_length = (uint)strlen(zuv.import_use_extension);
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this line is better to be
zuv.import_use_extension_length = sizeof(".php") - 1;

We use sizeof("str") - 1, cases like this.

Copy link
Author

Choose a reason for hiding this comment

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

The reason I did what I did, is because the only other usage of zuv.iport_use_extension also uses the strlen:

./Zend/zend.c:800: zend_uv.import_use_extension_length = (uint)strlen(zend_uv.import_use_extension);

Up to you/devs as to how it should be done, though.

@yohgaki
Copy link
Contributor

yohgaki commented Jan 22, 2015

Nice work!
Note for UPGRADING would be appreciated.

@MegaManSec
Copy link
Author

@yohgaki I don't know what to put in UPGRADING file, sorry.

@php-pulls php-pulls merged commit 6621840 into php:master Feb 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants