Skip to content

Converting some warnings to errors in file.c #5007

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 2 commits into from
Closed

Converting some warnings to errors in file.c #5007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78e06fc
Convert some warnings into ValueErrors in standard file.c
Girgias Dec 11, 2019
18f25a3
Use RETURN_THROWS();
Girgias Jan 7, 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
40 changes: 20 additions & 20 deletions ext/standard/file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ PHP_FUNCTION(flock)

act = operation & 3;
if (act < 1 || act > 3) {
php_error_docref(NULL, E_WARNING, "Illegal operation argument");
RETURN_FALSE;
zend_value_error("Illegal operation argument");
RETURN_THROWS();
}

if (wouldblock) {
Expand Down Expand Up @@ -745,8 +745,8 @@ PHP_FUNCTION(file)
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)) {
php_error_docref(NULL, E_WARNING, "'" ZEND_LONG_FMT "' flag is not supported", flags);
RETURN_FALSE;
zend_value_error("'" ZEND_LONG_FMT "' flag is not supported", flags);
RETURN_THROWS();
}

use_include_path = flags & PHP_FILE_USE_INCLUDE_PATH;
Expand Down Expand Up @@ -1040,8 +1040,8 @@ PHPAPI PHP_FUNCTION(fgets)
efree(buf);
} else if (argc > 1) {
if (len <= 0) {
php_error_docref(NULL, E_WARNING, "Length parameter must be greater than 0");
RETURN_FALSE;
zend_value_error("Length parameter must be greater than 0");
RETURN_THROWS();
}

str = zend_string_alloc(len, 0);
Expand Down Expand Up @@ -1495,8 +1495,8 @@ PHP_NAMED_FUNCTION(php_if_ftruncate)
ZEND_PARSE_PARAMETERS_END();

if (size < 0) {
php_error_docref(NULL, E_WARNING, "Negative size is not supported");
RETURN_FALSE;
zend_value_error("Negative size is not supported");
RETURN_THROWS();
}

PHP_STREAM_TO_ZVAL(stream, fp);
Expand Down Expand Up @@ -1750,8 +1750,8 @@ PHPAPI PHP_FUNCTION(fread)
PHP_STREAM_TO_ZVAL(stream, res);

if (len <= 0) {
php_error_docref(NULL, E_WARNING, "Length parameter must be greater than 0");
RETURN_FALSE;
zend_value_error("Length parameter must be greater than 0");
RETURN_THROWS();
}

str = php_stream_read_to_str(stream, len);
Expand Down Expand Up @@ -1829,8 +1829,8 @@ PHP_FUNCTION(fputcsv)
if (delimiter_str != NULL) {
/* Make sure that there is at least one character in string */
if (delimiter_str_len < 1) {
php_error_docref(NULL, E_WARNING, "delimiter must be a character");
RETURN_FALSE;
zend_value_error("delimiter must be a character");
Copy link
Member

Choose a reason for hiding this comment

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

(Drive-by) This message is somewhat confusing, maybe "delimiter cannot be empty"?

Copy link
Member

Choose a reason for hiding this comment

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

That message would better reflect the check, but actually only the first char is ever used as delimiter; maybe it's worth introducing a pontetial BC break here, and check for delimiter_str_len == 1)? Same for enclosure a few lines below.

Copy link
Member

Choose a reason for hiding this comment

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

Another possibility would be to actually add support for multiple chars (effectively: Unicode support).

Copy link
Member Author

Choose a reason for hiding this comment

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

Another possibility would be to actually add support for multiple chars (effectively: Unicode support).

Tried to have a look, it seems that the whole fgetcsv and fputcsv sub-routines need to be rewritten. I don't mind either way to merge the checks or have them adapted to have multibyte support. But I feel this should rather be in a separate PR.

RETURN_THROWS();
} else if (delimiter_str_len > 1) {
php_error_docref(NULL, E_NOTICE, "delimiter must be a single character");
Copy link
Member

Choose a reason for hiding this comment

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

I think it's important that we keep these pairs of warnings symmetrical. Either both should throw or neither.

Copy link
Member Author

Choose a reason for hiding this comment

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

Symmetrical with the SPL ones right? I'll do the changes at the same time borrowing parts from #4991

Copy link
Member

Choose a reason for hiding this comment

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

Symmetrical with the error above. I now realize though that in this case we throw a warning but still continue running the function, ugh. I think @cmb69's suggestion above to make this a single check (with BC break) may make the most sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah ok, want me to make that part of this PR or seperate one?

}
Expand All @@ -1841,8 +1841,8 @@ PHP_FUNCTION(fputcsv)

if (enclosure_str != NULL) {
if (enclosure_str_len < 1) {
php_error_docref(NULL, E_WARNING, "enclosure must be a character");
RETURN_FALSE;
zend_value_error("enclosure must be a character");
RETURN_THROWS();
} else if (enclosure_str_len > 1) {
php_error_docref(NULL, E_NOTICE, "enclosure must be a single character");
}
Expand Down Expand Up @@ -1967,8 +1967,8 @@ PHP_FUNCTION(fgetcsv)
if (delimiter_str != NULL) {
/* Make sure that there is at least one character in string */
if (delimiter_str_len < 1) {
php_error_docref(NULL, E_WARNING, "delimiter must be a character");
RETURN_FALSE;
zend_value_error("delimiter must be a character");
RETURN_THROWS();
} else if (delimiter_str_len > 1) {
php_error_docref(NULL, E_NOTICE, "delimiter must be a single character");
}
Expand All @@ -1979,8 +1979,8 @@ PHP_FUNCTION(fgetcsv)

if (enclosure_str != NULL) {
if (enclosure_str_len < 1) {
php_error_docref(NULL, E_WARNING, "enclosure must be a character");
RETURN_FALSE;
zend_value_error("enclosure must be a character");
RETURN_THROWS();
} else if (enclosure_str_len > 1) {
php_error_docref(NULL, E_NOTICE, "enclosure must be a single character");
}
Expand All @@ -2004,8 +2004,8 @@ PHP_FUNCTION(fgetcsv)
if (len_zv != NULL && Z_TYPE_P(len_zv) != IS_NULL) {
len = zval_get_long(len_zv);
if (len < 0) {
php_error_docref(NULL, E_WARNING, "Length parameter may not be negative");
RETURN_FALSE;
zend_value_error("Length parameter may not be negative");
RETURN_THROWS();
} else if (len == 0) {
len = -1;
}
Expand Down
11 changes: 7 additions & 4 deletions ext/standard/tests/file/bug71882.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ Bug #71882 (Negative ftruncate() on php://memory exhausts memory)
--FILE--
<?php
$fd = fopen("php://memory", "w+");
var_dump(ftruncate($fd, -1));
try {
var_dump(ftruncate($fd, -1));
Copy link
Member

Choose a reason for hiding this comment

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

All the var_dump()s in try-catch blocks could be removed as it's sure that an exception is thrown before executing them. At least this is how I prefer to do it, so feel free to ignore. Otherwise, the PR looks good for me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I don't like removing them is if we remove the Error/Exception you need to go back up and add it to make sure that the behaviour is expected.

Also everything is just "standard". Moreover, this can be removed after the fact.

} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}
?>
--EXPECTF--
Warning: ftruncate(): Negative size is not supported in %s%ebug71882.php on line %d
bool(false)
--EXPECT--
Negative size is not supported
2 changes: 2 additions & 0 deletions ext/standard/tests/file/fgetcsv_error_conditions.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
"water",fruit
This is line of text without csv fields
61 changes: 61 additions & 0 deletions ext/standard/tests/file/fgetcsv_error_conditions.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
--TEST--
Various fgetcsv() error conditions
--FILE--
<?php

$file_name = __DIR__ . '/fgetcsv_error_conditions.csv';
$file_handle = fopen($file_name, 'r');

$length = 1024;
$delimiter = ',';
$enclosure = '"';

echo 'fgetcsv() with negative length' . \PHP_EOL;
try {
var_dump( fgetcsv($file_handle, -10) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}
try {
var_dump( fgetcsv($file_handle, -10, $delimiter) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}
try {
var_dump( fgetcsv($file_handle, -10, $delimiter, $enclosure) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

echo 'fgetcsv() with delimiter as NULL' . \PHP_EOL;
try {
var_dump( fgetcsv($file_handle, $length, NULL, $enclosure) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

echo 'fgetcsv() with enclosure as NULL' . \PHP_EOL;
try {
var_dump( fgetcsv($file_handle, $length, $delimiter, NULL) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}

echo 'fgetcsv() with delimiter & enclosure as NULL' . \PHP_EOL;
try {
var_dump( fgetcsv($file_handle, $length, NULL, NULL) );
} catch (\ValueError $e) {
echo $e->getMessage() . \PHP_EOL;
}
?>
--EXPECT--
fgetcsv() with negative length
Length parameter may not be negative
Length parameter may not be negative
Length parameter may not be negative
fgetcsv() with delimiter as NULL
delimiter must be a character
fgetcsv() with enclosure as NULL
enclosure must be a character
fgetcsv() with delimiter & enclosure as NULL
delimiter must be a character
Loading