Fix GH-18753: file_get_contents() and file_put_contents() fail with data >=2GB on macOS & BSD
#18755
Conversation
|
Please target the |
…h data >=2GB on macOS & BSD
|
I just realized the problem also affects |
kiler129
changed the title
file_get_contents() and file_put_contents() fail with data >=2GB on macOS & BSD
| @@ -53,12 +53,16 @@ extern int php_get_uid_by_name(const char *name, uid_t *uid); | |||
| extern int php_get_gid_by_name(const char *name, gid_t *gid); | |||
| #endif | |||
|
|
|||
| #if defined(PHP_WIN32) | |||
| #if defined(PHP_WIN32) || defined(__APPLE__) || defined(__MACH__) || defined(BSD) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) | |||
There was a problem hiding this comment.
Is it possible to do a configure-time check for this?
There was a problem hiding this comment.
I think it might be just better to invert and allow it only for Linux
There was a problem hiding this comment.
I'm not really sure if Solaris can handle it either (can't be bother to check) so safest option should be to just do #ifdef __linux__ and invert the logic...
There was a problem hiding this comment.
It would probably be salient to also add a comment explaining why then. (the BSD and Windows cause should explain pretty much all cases).
(FWIW, AIX does seem to allow reads as big as off_t on 64-bit systems...)
| @@ -53,12 +53,16 @@ extern int php_get_uid_by_name(const char *name, uid_t *uid); | |||
| extern int php_get_gid_by_name(const char *name, gid_t *gid); | |||
| #endif | |||
|
|
|||
| #if defined(PHP_WIN32) | |||
| #if defined(PHP_WIN32) || defined(__APPLE__) || defined(__MACH__) || defined(BSD) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) | |||
There was a problem hiding this comment.
I think it might be just better to invert and allow it only for Linux
| --FILE-- | ||
| <?php | ||
| echo "-- file_put_contents() --\n"; | ||
| echo file_put_contents('bigfile', str_repeat('a', 2 ** 31)); |
There was a problem hiding this comment.
As it's quite a lot memory, I wonder if we should add some skip if there is not enough system memory. But it needs to work on Mac to be actually useful and not like https://github.com/php/php-src/blob/91becb304286942337ad65ec614cc0d1a3f79cd6/ext/standard/tests/file/file_get_contents_file_put_contents_5gb.phpt which is actually the reason why we didn't notice this (that test is always skipped on OSX and not sure if it works on FreeBSD either.
| --FILE-- | ||
| <?php | ||
| echo "-- file_put_contents() --\n"; | ||
| echo file_put_contents('bigfile', str_repeat('a', 2 ** 31)); |
There was a problem hiding this comment.
the name needs to be test specific - something like gh18753.data for example
| @@ -53,12 +53,16 @@ extern int php_get_uid_by_name(const char *name, uid_t *uid); | |||
| extern int php_get_gid_by_name(const char *name, gid_t *gid); | |||
| #endif | |||
|
|
|||
| #if defined(PHP_WIN32) | |||
| #if defined(PHP_WIN32) || defined(__APPLE__) || defined(__MACH__) || defined(BSD) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) | |||
There was a problem hiding this comment.
I'm not really sure if Solaris can handle it either (can't be bother to check) so safest option should be to just do #ifdef __linux__ and invert the logic...
|
I did bother to try on illumos yesterday, I forgot to tell but it works without the change. However I can t vouch for solaris which starts to be more and more behind as the time goes so I would be in favor to do what @bukka said, much simpler too. |
Problem
On Linux
read(3)syscall returns as many bytes as possible, up to requested maximum. Darwin/XNU and BSD kernels instead immediately returnsEINVALif the requested value is larger than nativeINT_MAX. This causesfile_get_contents()to fail with files larger than 2147483647 bytes.Fix
This patch applies the same clamping of buffer/read chunk to 2GB as previously on Windows. This fixes the
file_get_contents(). There's still a way to trigger the problem usingfread()withstream_set_read_buffer()set to 0/unbuffered, but I think this is more of an expected behavior. After this patch the later returns withEBADFinstead ofEINVAL.Affected version
The bug stems affects 8.2 onwards d/t 6beee1a from #8547. However, as this is not a security issue I'm targeting 8.3 as it's latest actively supported version.