Skip to content
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

FIX file_get_contents() on Windows fails with "errno=22 Invalid argument" #13948

Closed
wants to merge 4 commits into from

Conversation

DnwK98
Copy link
Contributor

@DnwK98 DnwK98 commented Apr 12, 2024

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):

Read of 3866046082 bytes failed with errno=22 Invalid argument

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.

@DnwK98 DnwK98 requested a review from bukka as a code owner April 12, 2024 11:51
@mvorisek
Copy link
Contributor

mvorisek commented Apr 12, 2024

👍 documented in https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170

if buffer_size > INT_MAX, the invalid parameter handler is invoked

should target bugfix branch

@DnwK98 DnwK98 force-pushed the fix-file-get-content-windows branch from d6a1d9d to b48e85e Compare April 12, 2024 13:12
@DnwK98 DnwK98 changed the base branch from master to PHP-8.2.18 April 12, 2024 13:13
@DnwK98 DnwK98 changed the base branch from PHP-8.2.18 to PHP-8.2 April 12, 2024 13:14
@DnwK98
Copy link
Contributor Author

DnwK98 commented Apr 12, 2024

Targets PHP-8.2

@@ -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))
Copy link
Member

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 ?

@DnwK98 DnwK98 force-pushed the fix-file-get-content-windows branch from 80d9e27 to 9ef8bbe Compare April 12, 2024 14:05
@devnexen
Copy link
Member

I do not get your "revert" :) previous version was fine I think.

@DnwK98
Copy link
Contributor Author

DnwK98 commented Apr 12, 2024

It wasn't, as windows uses _read() and others systems uses read()

I also didn't see this at the beginning.

@devnexen
Copy link
Member

oh I did not see the inconsistency between _write/write and read

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

LCTM

Copy link
Member

@bukka bukka left a 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.

@DnwK98
Copy link
Contributor Author

DnwK98 commented Apr 12, 2024

I updated test created in previous ticket, but I'm also not able to run it against Windows.
image

@bukka
Copy link
Member

bukka commented Apr 12, 2024

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)?

@DnwK98
Copy link
Contributor Author

DnwK98 commented Apr 13, 2024

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.
image

Copy link
Member

@bukka bukka left a 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

@bukka bukka closed this in 8421cfd Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants