Skip to content

Fix #81145: copy() and stream_copy_to_stream() fail for +4GB files #7158

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

Closed
wants to merge 5 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 16, 2021

When mapping the file, we need to pass the proper dwFileOffsetHigh
instead of 0.


I'm not sure about adding a regression test. That would be terribly slow, but given that I missed this issue when fixing bug 79423 it might make sense to have one.

When mapping the file, we need to pass the proper `dwFileOffsetHigh`
instead of `0`.
@cmb69 cmb69 added the Bug label Jun 16, 2021
@nikic
Copy link
Member

nikic commented Jun 16, 2021

I'm not sure about adding a regression test. That would be terribly slow, but given that I missed this issue when fixing bug 79423 it might make sense to have one.

How slow are we talking here? On Linux the test case takes 2s, is it much slower on Windows?

@cmb69
Copy link
Member Author

cmb69 commented Jun 16, 2021

How slow are we talking here? On Linux the test case takes 2s, is it much slower on Windows?

Takes ~50sec for me. Lets see on CI. (see also https://bugs.php.net/bug.php?id=80695)

@cmb69
Copy link
Member Author

cmb69 commented Jun 16, 2021

Well, seems perf depends: Linux (16s) vs Windows (9s). Anyway, the test makes sense.

@divinity76
Copy link
Contributor

divinity76 commented Jun 16, 2021

FWIW we didn't add a test for https://bugs.php.net/bug.php?id=80523 either
(that bug required ~80GB RAM and ~4GB disk space to test!)

Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
@nikic
Copy link
Member

nikic commented Jun 17, 2021

Test fails on 32-bit Linux:

Warning: fopen(/home/vsts/work/1/s/ext/standard/tests/file/bug81145_src.bin): failed to open stream: Value too large for defined data type

We can't copy files > 4GB there, anyway.
@cmb69
Copy link
Member Author

cmb69 commented Jun 17, 2021

Ah, thanks! Need to skip there.

@cmb69
Copy link
Member Author

cmb69 commented Jun 17, 2021

@divinity76, PHP currently only supports some 32bit and 64bit architectures. To support other architectures, would require many more changes, so adapting this test as well shouldn't be hard (there are already ~200 other tests with basically the same SKIPIF clause).

@cmb69 cmb69 closed this in 2555efa Jun 17, 2021
@cmb69 cmb69 deleted the cmb/79423 branch June 17, 2021 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants