Skip to content

Make is_file() and friends return false when path contains 0-byte #6478

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.

Sign up for GitHub

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
wants to merge 4 commits into from
Closed

Make is_file() and friends return false when path contains 0-byte #6478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7fb19e1
Make is_file() and friends return false when path contains 0-byte
Dec 2, 2020
b16fd33
Use standard way of checking for 0-bytes in path
Dec 2, 2020
6bcd692
Adapt tests to FileFunction returning false on 0-bytes
Dec 2, 2020
29cb3af
Make warnings for filenames containing null byte consistent with non-…
Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ext/standard/filestat.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,10 @@ 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)) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (filename_length && !IS_EXISTS_CHECK(type)) {
php_error_docref(NULL, E_WARNING, "Filename contains null byte");
}
RETURN_FALSE;
}

Expand Down Expand Up @@ -937,7 +940,7 @@ ZEND_NAMED_FUNCTION(name) { \
size_t filename_len; \
\
ZEND_PARSE_PARAMETERS_START(1, 1) \
Z_PARAM_PATH(filename, filename_len) \
Z_PARAM_STRING(filename, filename_len) \
ZEND_PARSE_PARAMETERS_END(); \
\
php_stat(filename, filename_len, funcnum, return_value); \
Expand Down
9 changes: 2 additions & 7 deletions ext/standard/tests/file/bug39863.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ Andrew van der Stock, vanderaj @ owasp.org
<?php

$filename = __FILE__ . chr(0). ".ridiculous";

try {
var_dump(file_exists($filename));
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}
var_dump(file_exists($filename));
?>
--EXPECT--
file_exists(): Argument #1 ($filename) must not contain any null bytes
bool(false)
8 changes: 6 additions & 2 deletions ext/standard/tests/file/filegroup_variation3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,12 @@ bool(false)
Warning: filegroup(): stat failed for %s/filegroup_variation3/filegroup*.tmp in %s on line %d
bool(false)
- Iteration 7 -
filegroup(): Argument #1 ($filename) must not contain any null bytes

Warning: filegroup(): Filename contains null byte in %s on line %d
bool(false)
- Iteration 8 -
filegroup(): Argument #1 ($filename) must not contain any null bytes

Warning: filegroup(): Filename contains null byte in %s on line %d
bool(false)

*** Done ***
8 changes: 6 additions & 2 deletions ext/standard/tests/file/fileinode_variation3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,12 @@ bool(false)
Warning: fileinode(): stat failed for %s/fileinode_variation3/fileinode*.tmp in %s on line %d
bool(false)
- Iteration 7 -
fileinode(): Argument #1 ($filename) must not contain any null bytes

Warning: fileinode(): Filename contains null byte in %s on line %d
bool(false)
- Iteration 8 -
fileinode(): Argument #1 ($filename) must not contain any null bytes

Warning: fileinode(): Filename contains null byte in %s on line %d
bool(false)

*** Done ***
8 changes: 6 additions & 2 deletions ext/standard/tests/file/fileowner_variation3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,12 @@ bool(false)
Warning: fileowner(): stat failed for %s/fileowner_variation3/fileowner*.tmp in %s on line %d
bool(false)
- Iteration 7 -
fileowner(): Argument #1 ($filename) must not contain any null bytes

Warning: fileowner(): Filename contains null byte in %s on line %d
bool(false)
- Iteration 8 -
fileowner(): Argument #1 ($filename) must not contain any null bytes

Warning: fileowner(): Filename contains null byte in %s on line %d
bool(false)

*** Done ***
8 changes: 6 additions & 2 deletions ext/standard/tests/file/fileperms_variation3.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,12 @@ bool(false)
Warning: fileperms(): stat failed for %s/fileperms_variation3/fileperms*.tmp in %s on line %d
bool(false)
- Iteration 7 -
fileperms(): Argument #1 ($filename) must not contain any null bytes

Warning: fileperms(): Filename contains null byte in %s on line %d
bool(false)
- Iteration 8 -
fileperms(): Argument #1 ($filename) must not contain any null bytes

Warning: fileperms(): Filename contains null byte in %s on line %d
bool(false)

*** Done ***
4 changes: 2 additions & 2 deletions ext/standard/tests/file/is_dir_variation4.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ bool(true)
bool(false)

-- Iteration 9 --
is_dir(): Argument #1 ($filename) must not contain any null bytes
bool(false)

-- Iteration 10 --
is_dir(): Argument #1 ($filename) must not contain any null bytes
bool(false)

*** Done ***
4 changes: 2 additions & 2 deletions ext/standard/tests/file/is_executable_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ bool(false)
-- Iteration 5 --
bool(false)
-- Iteration 6 --
is_executable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
-- Iteration 7 --
is_executable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
-- Iteration 8 --
bool(false)
-- Iteration 9 --
Expand Down
4 changes: 2 additions & 2 deletions ext/standard/tests/file/is_file_variation4.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ bool(false)
- Iteration 6 -
bool(false)
- Iteration 7 -
is_file(): Argument #1 ($filename) must not contain any null bytes
bool(false)
- Iteration 8 -
is_file(): Argument #1 ($filename) must not contain any null bytes
bool(false)

*** Done ***
6 changes: 3 additions & 3 deletions ext/standard/tests/file/is_readable_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,11 @@ bool(false)
-- Iteration 6 --
bool(false)
-- Iteration 7 --
is_readable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
-- Iteration 8 --
is_readable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
-- Iteration 9 --
is_readable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
-- Iteration 10 --
bool(true)
-- Iteration 11 --
Expand Down
12 changes: 6 additions & 6 deletions ext/standard/tests/file/is_writable_variation1.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ bool(false)
bool(false)
bool(false)
-- Iteration 7 --
is_writable(): Argument #1 ($filename) must not contain any null bytes
is_writeable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
bool(false)
-- Iteration 8 --
is_writable(): Argument #1 ($filename) must not contain any null bytes
is_writeable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
bool(false)
-- Iteration 9 --
is_writable(): Argument #1 ($filename) must not contain any null bytes
is_writeable(): Argument #1 ($filename) must not contain any null bytes
bool(false)
bool(false)
-- Iteration 10 --
bool(true)
bool(true)
Expand Down