Skip to content

Remove include "sanity check" to get better error #19650

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

Merged
merged 2 commits into from
Aug 31, 2025
+48 −26
Merged

Remove include "sanity check" to get better error #19650

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
c9923a6
Remove include "sanity check" to get better error
nikic Aug 31, 2025
04f5618
Update error message to include include_path
krakjoe Aug 31, 2025
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
21 changes: 21 additions & 0 deletions Zend/tests/require_directory.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
Including a directory generates an error
--SKIPIF--
<?php
if (PHP_OS_FAMILY === 'Windows') die("skip Not for Windows");
?>
--FILE--
<?php

/* Just a random test directory */
$dir = __DIR__ . '/variadic';
require $dir;

?>
--EXPECTF--
Notice: require(): Read of %d bytes failed with errno=21 Is a directory in %s on line %d

Fatal error: Uncaught Error: Failed opening required '%s' (include_path='.:') in %s:%d
Stack trace:
#0 {main}
thrown in %s on line %d
21 changes: 21 additions & 0 deletions Zend/tests/require_directory_windows.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
--TEST--
Including a directory generates an error (Windows variant)
--SKIPIF--
<?php
if (PHP_OS_FAMILY !== 'Windows') die("skip Only for Windows");
?>
--FILE--
<?php

/* Just a random test directory */
$dir = __DIR__ . '/variadic';
require $dir;

?>
--EXPECTF--
Warning: require(%s): Failed to open stream: Permission denied in %s on line %d

Fatal error: Uncaught Error: Failed opening required '%s' (include_path='%s') in %s:%d
Stack trace:
#0 {main}
thrown in %s on line %d
6 changes: 5 additions & 1 deletion ext/standard/tests/file/bug35740.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
--TEST--
Bug #35740 (memory leak when including a directory)
--SKIPIF--
<?php
if (PHP_OS_FAMILY === 'Windows') die("skip Not for Windows");
?>
--FILE--
<?php

Expand All @@ -8,7 +12,7 @@ include (__DIR__);
echo "Done\n";
?>
--EXPECTF--
Warning: include(%s): Failed to open stream: %s in %s on line %d
Notice: include(): Read of %s bytes failed with errno=21 Is a directory in %s on line %d
Copy link
Member

Choose a reason for hiding this comment

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

We just had an internal discussion about this error downgrade which is happening due to error triggered during read which is notice. The decision is to revert this for now. I'm looking to error changes so might be able to handle this better.

It's not probably huge issue but might be quite late for this and it's worth to delay it. I'm also not sure if the Win inconsistency is a good thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that only the first of two warnings gets downgraded. The second one (Failed opening '%s' for inclusion) is always a warning and consistent across OS.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I noticed that - that's why I don't think it's a huge issue. I was thinking that it would be better to keep warning but more I think about it, this might seem possibly better. But it's still not nice that we need different test for Windows. I'm starting to think that the stat might be there to make it consistent so maybe checking how to get more detailed error on Windows as well would be ideal.

It's well past the feature freeze so I still think it's better to delay it as it needs a bit more considering.


Warning: include(): Failed opening '%s' for inclusion (include_path='%s') in %s on line %d
Done
26 changes: 1 addition & 25 deletions main/streams/plain_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ typedef struct {
unsigned is_pipe:1; /* stream is an actual pipe, currently Windows only*/
unsigned cached_fstat:1; /* sb is valid */
unsigned is_pipe_blocking:1; /* allow blocking read() on pipes, currently Windows only */
unsigned no_forced_fstat:1; /* Use fstat cache even if forced */
unsigned is_seekable:1; /* don't try and seek, if not set */
unsigned _reserved:26;

Expand All @@ -161,7 +160,7 @@ typedef struct {

static int do_fstat(php_stdio_stream_data *d, int force)
{
if (!d->cached_fstat || (force && !d->no_forced_fstat)) {
if (!d->cached_fstat || force) {
int fd;
int r;

Expand Down Expand Up @@ -1188,30 +1187,7 @@ PHPAPI php_stream *_php_stream_fopen(const char *filename, const char *mode, zen
efree(persistent_id);
}

/* WIN32 always set ISREG flag */
#ifndef PHP_WIN32
/* sanity checks for include/require.
* We check these after opening the stream, so that we save
* on fstat() syscalls */
if (options & STREAM_OPEN_FOR_INCLUDE) {
php_stdio_stream_data *self = (php_stdio_stream_data*)ret->abstract;
int r;

r = do_fstat(self, 0);
if ((r == 0 && !S_ISREG(self->sb.st_mode))) {
if (opened_path) {
zend_string_release_ex(*opened_path, 0);
*opened_path = NULL;
}
php_stream_close(ret);
return NULL;
}

/* Make sure the fstat result is reused when we later try to get the
* file size. */
self->no_forced_fstat = 1;
}

if (options & STREAM_USE_BLOCKING_PIPE) {
php_stdio_stream_data *self = (php_stdio_stream_data*)ret->abstract;
self->is_pipe_blocking = 1;
Expand Down