Make is_file() and friends return false when path contains 0-byte #6478
Conversation
Full list of affected functions: is_writable, is_readable, is_executable, is_file, is_dir, is_link, fileperms, fileinode, filesize, fileowner, filegroup, filetype, fileatime, filemtime, filectime, file_exists, lstat, stat
|
This is going to need some test updates. They can likely be performed automatically: |
| @@ -728,7 +728,7 @@ PHPAPI void php_stat(const char *filename, size_t filename_length, int type, zva | |||
| const char *local; | |||
| php_stream_wrapper *wrapper; | |||
|
|
|||
| if (!filename_length) { | |||
| if (!filename_length || CHECK_NULL_PATH(filename, filename_length)) { | |||
There was a problem hiding this comment.
This looks basically fine, with one remaining concern: This code currently has different behavior for IS_EXISTS_CHECK and everything else. For non-EXISTS checks, a warning is emitted on stat failure:
sapi/cli/php -r 'stat("does_not_exist");'
Warning: stat(): stat failed for does_not_exist in Command line code on line 1
After this change, the behavior would be that stat("does_not_exist") prints a warning, but stat("does_not\0_exist") will silently return false, which seems inconsistent to me.
Might it make sense to add a warning in that case?
if (!IS_EXISTS_CHECK(type)) {
php_error_docref(NULL, E_WARNING, "File name is empty or contains null byte");
}
I'm not sure on this point though.
There was a problem hiding this comment.
I thought about that too. I was never the biggest fan of those warnings but as they have some merit it might make sense to increase consistency in the 0-byte case.
I think I'll add your suggested warning.
There was a problem hiding this comment.
Maybe the warning should only trigger when there is a 0-byte as an empty string/null didn't trigger a warning before and it is a very minor BC break to add the warning in that case.
Does this have to be discussed?
There was a problem hiding this comment.
For me, It is a bug that stat("") does not trigger a warning but returns false. Whenever I use this function without prefixing it by @, I expect that it will either return an array, or run the panic procedure of my error handler.
There was a problem hiding this comment.
That is a separate issue and I did not want to change the behavior for null/empty string along with the 0-byte change to not introduce new warnings as a side-effect.
But I see you reasoning and I guess it could be discussed in a separate thread.
|
shouln't this go into |
Full list of affected functions:
is_writable, is_readable, is_executable, is_file, is_dir, is_link,
fileperms, fileinode, filesize, fileowner, filegroup, filetype,
fileatime, filemtime, filectime,
file_exists, lstat, stat