fix GH-13203 file_put_contents fail on strings over 4GB on Windows #13205

divinity76 · 2024-01-20T18:39:03Z

used 10GB in SKIPIF to account for people running run-tests.php in parallel; i want there to be a comfortable amount of system resources, don't want to run if it's just barely enough.

actually exist a macro to do the cast (not implemented exactly how I would have preferred, but it's fine. is UINT_MAX actually guaranteed to be unsigned int? does specifications actually dictate that, or did the compiler developers just chose to implement it like that?)

SakiTakamachi · 2024-01-23T13:07:11Z

I'm not very familiar with that, but did this solve the problem?
I was concerned because the behavior does not seem to change depending on PLAIN_WRAP_BUF_SIZE on the 64-bit version of Windows.

divinity76 · 2024-01-23T14:29:08Z

@SakiTakamachi don't actually know. Was hoping WINDOWS_X64_ZTS CI tester would tell me, but I can't actually see if the test passed or was skipped due to system resource constraints,
i can only see that it did not fail.

I tried getting MSVC to work, but that's seemingly non-trivial: MSVC2022 is not yet supported, and I would need to download MSVC2017 or something to actually compile PHP on MSVC? I gave up. Hopefully a developer actually equipped with MSVC can test it

SakiTakamachi · 2024-01-23T14:39:11Z

I see, if I can look it this week, I'll check this out.

nielsdos · 2024-01-23T16:32:52Z

I'm familiar with doing Windows debugs and I have a dedicated VM to do so, so I can also check tonight

bukka · 2024-01-23T16:39:02Z

It might be potentially better to enable chunking for big writes so we don't need to write it all in one go.

nielsdos · 2024-01-23T20:17:15Z

I was concerned because the behavior does not seem to change depending on PLAIN_WRAP_BUF_SIZE on the 64-bit version of Windows.

PHP_WIN32 is also set on 64-bit Windows. This is because Win32 is actually the name of the subsystem, not the bitness.
So the macro actually does change something.
_WIN64 is the macro only set on 64-bit Windows.

nielsdos · 2024-01-23T20:28:32Z

Just checked, CI skipped your test.

nielsdos · 2024-01-23T20:33:41Z

I tried testing this, but my VM just dies when trying to do large I/O, so someone else will have to ¯_(ツ)_/¯

bukka · 2024-01-23T22:25:13Z

I have a real machine so I can take a look later this week.

SakiTakamachi · 2024-01-23T22:33:40Z

@nielsdos
Thanks, but if you look a little above the changed line, it appears to be doing almost the same thing as the added macro, so I feel like the behavior is still the same.

divinity76 · 2024-01-23T22:36:57Z

good point.. @nielsdos could you check if your VM hits the

		if (ZEND_SIZE_T_UINT_OVFL(count)) {
			count = UINT_MAX;
		}

condition on line 359? maybe the real issue is in ZEND_SIZE_T_UINT_OVFL() failing? or maybe i'm looking in the wrong place completely

nielsdos · 2024-01-23T23:06:13Z

Yes that condition is hit and the body is executed.
EDIT: and indeed this patch does not work.

divinity76 · 2024-01-23T23:10:21Z

@nielsdos try again now.

can actually partially reproduce the issue with just

#include <iostream>
#include <stdio.h>
#include <stdint.h>
#include <BaseTsd.h>
#include <io.h>
#include<thread>
#include<chrono>
int main()
{
    FILE* h;
    tmpfile_s(&h);
    int h_int = _fileno(h);
    std::string gb5(size_t(5) * 1024 * 1024 * 1024, 'a');
    while (gb5.length() > 0) {
        int written_now = _write(h_int, gb5.data(), gb5.length() > UINT_MAX ? UINT_MAX-5 : gb5.length());   
        std::cout << "written_now" << written_now << std::endl;
        gb5.erase(0, written_now);
        std::this_thread::sleep_for(std::chrono::seconds(3));
    }
}

basically microsoft's _write function becomes unstable somewhere above INT_MAX

nielsdos · 2024-01-23T23:14:35Z

The patch currently works.
It's not that it becomes unstable, it's peak idiotic API design actually: the _write function returns an int. So when the size becomes larger than INT_MAX, the return value becomes negative which is confused for an error that happened because error codes are also negative...

divinity76 · 2024-01-23T23:18:21Z

The patch currently works. It's not that it becomes unstable, it's peak idiotic API design actually: the _write function returns an int. So when the size becomes larger than INT_MAX, the return value becomes negative which is confused for an error that happened because error codes are also negative...

so basically, if you try to use _write() above INT_MAX, it becomes impossible to differentiate between error and success 😵‍

nielsdos · 2024-01-23T23:19:05Z

The following works too (tested), and do less system calls probably:

$ git diff main\streams\plain_wrapper.c
diff --git a/main/streams/plain_wrapper.c b/main/streams/plain_wrapper.c
index e9a30f3334..c5aa807c4f 100644
--- a/main/streams/plain_wrapper.c
+++ b/main/streams/plain_wrapper.c
@@ -358,10 +358,11 @@ static ssize_t php_stdiop_write(php_stream *stream, const char *buf, size_t coun
			count = UINT_MAX;
		}
		bytes_written = _write(data->fd, buf, (unsigned int)count);
+		if (errno != 0) {
 #else
		ssize_t bytes_written = write(data->fd, buf, count);
-#endif
		if (bytes_written < 0) {
+#endif
			if (PHP_IS_TRANSIENT_ERROR(errno)) {
				return 0;
			}

nielsdos · 2024-01-23T23:20:19Z

so basically, if you try to use _write() above INT_MAX, it becomes impossible to differentiate between error and success 😵‍

Indeed, a real footgun. It's possible to check errno instead but yeah...

divinity76 · 2024-01-23T23:24:29Z

bytes_written = _write(data->fd, buf, (unsigned int)count);
+		if (errno != 0) {

makes me uneasy, bytes_written may be negative and then return bytes_written; may return negative rather than positive,
and then the whole php_stdiop_write function may return negative values for a successful write, that will probably break something further up 🤔

nielsdos · 2024-01-23T23:26:57Z

Yeah that's true. And would also cause problems in 32-bit where sizeof(ssize_t) == sizeof(int)...
I think your patch as-is is fine. Not sure about the heavy test case in CI, but deferring the final say to Jakub.

SakiTakamachi · 2024-01-24T00:27:27Z

It seemed like there was a lot of discussion going on while I was getting ready in the morning.
I'm not very familiar with it so my opinion doesn't really matter, but it looks good to me.

bukka

Looks reasonable, just test needs to not use tmpfile and one NIT possibly.

main/streams/plain_wrapper.c