/ext/standard: Check for empty string in linkinfo()#21793
/ext/standard: Check for empty string in linkinfo()#21793LamentXU123 wants to merge 1 commit intophp:masterfrom
Conversation
| @@ -84,7 +84,11 @@ PHP_FUNCTION(linkinfo) | |||
| Z_PARAM_PATH(link, link_len) | |||
| ZEND_PARSE_PARAMETERS_END(); | |||
|
|
|||
| // TODO Check for empty string | |||
| if (UNEXPECTED(link_len == 0)) { | |||
| php_error_docref(NULL, E_WARNING, "%s", strerror(ENOENT)); | |||
There was a problem hiding this comment.
I would go for a ValueError exception, i.e. zend_argument_must_not_be_empty_error(1). Also you might need at least one test.
There was a problem hiding this comment.
There are existing tests, see https://github.com/php/php-src/blob/master/ext/standard/tests/file/symlink_link_linkinfo_is_link_error1.phpt#L30
Originally it raise a warning and return int(-1), and I don't have strong opinions on whether we should change the behavior here. The initial idea is just to add a fast guard explicitly for the case as per the TODO message.
There was a problem hiding this comment.
That is a valid point, it is just the direction most of extensions lean towards but I ll leave the decision to @bukka on this one :)
There was a problem hiding this comment.
The new Policy RFC allows to just emit a ValueError. Ideally we'd be able to audit all the call sites and basically just mandate that the path passed to zend_dirname() must not be empty nor contain null bytes.
3 participants
If link_len == 0, it now emits the same warning style as the existing filesystem error path and returns -1 immediately, avoiding the later estrndup() / zend_dirname() work on an empty input as per the TODO message.