Skip to content

make FILE_ constants not collide with LOCK_ #11254

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+49 −193
Open

make FILE_ constants not collide with LOCK_ #11254

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
0b1e0e7
make FILE_ constants not collide with LOCK_
divinity76 May 17, 2023
312deeb
make file() flag check stricter + fix testcase
divinity76 May 17, 2023
b612dff
fix 2 more testcases
divinity76 May 17, 2023
bff8b26
Technically it's PHP_LOCK_
divinity76 May 19, 2023
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
3 changes: 2 additions & 1 deletion ext/standard/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,8 @@ PHP_FUNCTION(file)
Z_PARAM_RESOURCE_OR_NULL(zcontext)
ZEND_PARSE_PARAMETERS_END();

if (flags < 0 || flags > (PHP_FILE_USE_INCLUDE_PATH | PHP_FILE_IGNORE_NEW_LINES | PHP_FILE_SKIP_EMPTY_LINES | PHP_FILE_NO_DEFAULT_CONTEXT)) {
if ((flags & ~(PHP_FILE_USE_INCLUDE_PATH | PHP_FILE_IGNORE_NEW_LINES | PHP_FILE_SKIP_EMPTY_LINES | PHP_FILE_NO_DEFAULT_CONTEXT))
!= 0) {
zend_argument_value_error(2, "must be a valid flag value");
RETURN_THROWS();
}
Expand Down
10 changes: 5 additions & 5 deletions ext/standard/file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ PHPAPI ssize_t php_fputcsv(php_stream *stream, zval *fields, char delimiter, cha

#define META_DEF_BUFSIZE 8192

#define PHP_FILE_USE_INCLUDE_PATH (1 << 0)
#define PHP_FILE_IGNORE_NEW_LINES (1 << 1)
#define PHP_FILE_SKIP_EMPTY_LINES (1 << 2)
#define PHP_FILE_APPEND (1 << 3)
#define PHP_FILE_NO_DEFAULT_CONTEXT (1 << 4)
#define PHP_FILE_USE_INCLUDE_PATH (1 << 3) // avoid collision with the PHP_LOCK_ constants.
#define PHP_FILE_IGNORE_NEW_LINES (1 << 4)
#define PHP_FILE_SKIP_EMPTY_LINES (1 << 5)
#define PHP_FILE_APPEND (1 << 6)
#define PHP_FILE_NO_DEFAULT_CONTEXT (1 << 7)

typedef enum _php_meta_tags_token {
TOK_EOF = 0,
Expand Down
20 changes: 13 additions & 7 deletions ext/standard/tests/file/file_error.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,33 @@ Test file() function : error conditions
<?php
$file_path = __DIR__;
echo "\n*** Testing error conditions ***\n";
$file_handle = fopen($file_path."/file.tmp", "w");
$file_handle = fopen($file_path . "/file.tmp", "w");

$filename = $file_path."/file.tmp";
var_dump( file($filename, 10, NULL) ); // Incorrect flag
$filename = $file_path . "/file.tmp";
try {
var_dump(file($filename, 10, NULL)); // Incorrect flag
} catch (ValueError $e) {
echo "Exception: " . $e->getMessage() . "\n";
}

var_dump(file("temp.tmp")); // non existing filename

var_dump( file("temp.tmp") ); // non existing filename
fclose($file_handle);

echo "\n--- Done ---";

?>
--CLEAN--
<?php
$file_path = __DIR__;
unlink($file_path."/file.tmp");
?>
--EXPECTF--

*** Testing error conditions ***
array(0) {
}
Exception: file(): Argument #2 ($flags) must be a valid flag value

Warning: file(temp.tmp): Failed to open stream: No such file or directory in %s on line %d
Warning: file(temp.tmp): Failed to open stream: No such file or directory in %s on line 13
bool(false)

--- Done ---
2 changes: 1 addition & 1 deletion ext/standard/tests/file/file_variation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ ini_set( 'include_path',$file_path.'/file_variation' );

file_put_contents( $file_path1."/file1_variation.tmp", "aaaaaaaaaaaaaaabbbbbbbbbbb111111111222222222" );
var_dump( file("file1_variation.tmp", FILE_USE_INCLUDE_PATH) );
var_dump( file($file_path1."/file1_variation.tmp", 1) );
var_dump( file($file_path1."/file1_variation.tmp", FILE_USE_INCLUDE_PATH) );

echo "*** Using file function to remove line containing a key string ***\n";
$file_handle = fopen($file_path."/file2_variation.tmp", "w");
Expand Down
207 changes: 28 additions & 179 deletions ext/standard/tests/file/file_variation6.phpt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is wild. Let's totally just iterate over a range of ints as flag values and test the behavior 🤦🏻

But, that's not your fault. "Looks good to me."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe yeah, and thanks for checking! btw the old check was actually flawed and would miss some incorrect flags, for example flags = 10 aka flags= (1 << 1) | (1 << 3) is, and always was, incorrect for file() because file() doesn't support PHP_FILE_APPEND, but the old $flags error-check code would miss it. the new check doesn't. As an added bonus, seems the new flags check is about 7% faster too: https://quick-bench.com/q/j3k_unXcE91gVdTRH9Dm0nxpDUA
(probably because it doesn't do the flags < 0 check)

Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ array(3) {
[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
array(3) {
[0]=>
string(7) "Line 1
Expand All @@ -43,6 +50,13 @@ array(3) {
[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
array(3) {
[0]=>
string(6) "Line 1"
Expand All @@ -51,6 +65,13 @@ array(3) {
[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
array(3) {
[0]=>
string(6) "Line 1"
Expand All @@ -59,6 +80,13 @@ array(3) {
[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
array(3) {
[0]=>
string(7) "Line 1
Expand All @@ -69,182 +97,3 @@ array(3) {
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(7) "Line 1
"
[1]=>
string(7) "Line 2
"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
array(3) {
[0]=>
string(6) "Line 1"
[1]=>
string(6) "Line 2"
[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value
file(): Argument #2 ($flags) must be a valid flag value