-
Notifications
You must be signed in to change notification settings - Fork 7.9k
FIX file_get_contents() on Windows fails with "errno=22 Invalid argument" #13948
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.
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
Conversation
👍 documented in https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170
should target bugfix branch |
d6a1d9d
to
b48e85e
Compare
b48e85e
to
3802a30
Compare
Targets |
@@ -54,7 +54,7 @@ extern int php_get_gid_by_name(const char *name, gid_t *gid); | |||
#endif | |||
|
|||
#if defined(PHP_WIN32) | |||
# define PLAIN_WRAP_BUF_SIZE(st) (((st) > UINT_MAX) ? UINT_MAX : (unsigned int)(st)) | |||
# define PLAIN_WRAP_BUF_SIZE(st) ((unsigned int)(st > INT_MAX ? INT_MAX : st)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I think it is possible to simplify line 356/360, what do you think ?
80d9e27
to
9ef8bbe
Compare
I do not get your "revert" :) previous version was fine I think. |
It wasn't, as windows uses I also didn't see this at the beginning. |
oh I did not see the inconsistency between _write/write and read |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LCTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs a test - otherwise look ok.
PASS means that you were able to run it and it was fine. If it wasn't run, it would be a skip. Or is the screen shot not from Windows (the test should work also on Linux and other platforms with enough memory)? |
I don't have computer with Windows, also test skips on CI as was suggested in previous bug. You @bukka were person who ran test on Windows locally previously, so if you could also now, I'll be pleased. Edit: This test is not skipped on CI as was suggested, so I did test without fix and it failed as expected. After fix it succeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Windows and works fine
Got the same error when writing a huge file on macOS arm64 PHP 8.3.10
|
|
I encountered a problem when attempting to load a large file using file_get_contents(), but only on Windows.
Attempting to load a large file resulted in the following error (or notice, depending on the PHP version):
I did some research and found bug which was quite similar, but for
file_put_contents()
Old bug: #13205
This issue is likely due to an integer overflow on Windows, which probably is treated as negative number which is invalid argument.