stream: limit unix read/write to 2GB at a time#1501
stream: limit unix read/write to 2GB at a time#1501vtjnash wants to merge 3 commits intolibuv:v1.xfrom
Conversation
bnoordhuis
left a comment
There was a problem hiding this comment.
Learn something new every day. Thanks for the PR.
Since it's a MacOS-only issue, would it make sense to put the code in #ifdef guards?
Aside: a regression test would be nice!
I'm not sure. On the original issue, it was noted that it it's largely inconsequential to do a syscall every 2GB. Also, on Linux, we can see from testing that it doesn't actually do larger writes, it's simply just better at reporting: |
15852a9 to
1e99118
Compare
|
Please take another look, thanks! (While writing tests for this, I noticed uv_fs_write seems to have a similar bug, in particular, the implementation appears to assume that write can't fail for any reason (even EINTER), and it just continues with processing later buffers. But that will have to be addressed in a separate issue.) Lines 1010 to 1012 in 564677d |
|
Any reason not to target this at v1.x? |
|
Bump. Is there anything else to be done to get this merged? |
Hi @vtjnash this is already adressed in issue #640 |
|
@saghul @bnoordhuis The PR nodejs/node#11641 will need to be altered to work around this bug. Mind giving a status update as to whether this (or the other) fix will land? |
4c570e5 to
4b325df
Compare
|
This is updated now to merge into v1.x, in hopes that it'll get some review now. I've also discovered that libuv on NT can't handle writes this large (NT returns some sort of internal error code instead), so I've backed off on the max size on that platform (perhaps I should add a note to the docs that you shouldn't attempt to write more than about 2^30 bytes at a time?). |
I once suggested a public implementation for uv_getiovmax in libuv #802. |
|
Not really, since this isn't a limit on the number of iovcnt, and there is no maximum |
|
Understood. (Thanks for the explanation!) |
|
What's the buildbot configurations? A lot of them are failing to also a few failures that seem unrelated: |
|
Alright, I've tried a trick to get 32-bit machines to give me a large allocation. This won't solve the issue with the x64 build bot configuration (what is their configuration?), but will hopefully get a few more passing. |
|
I recommend the following change for z/OS. --- a/src/unix/internal.h
+++ b/src/unix/internal.h
@@ -30,10 +30,6 @@
#include <fcntl.h> /* O_CLOEXEC, may be */
#include <stdio.h>
-#ifndef INT32_MAX
-#define INT32_MAX (2147483647)
-#endif
-
#if defined(__STRICT_ANSI__)
# define inline __inline
#endif
diff --git a/uv.gyp b/uv.gyp
index 96fb801..72d18b4 100644
--- a/uv.gyp
+++ b/uv.gyp
@@ -28,7 +28,7 @@
'_AE_BIMODAL',
'PATH_MAX=255'
],
- 'cflags': [ '-qxplink' ],
+ 'cflags': [ '-qxplink', '-qlanglvl=extc99' ],
'ldflags': [ '-qxplink' ],
}]
], |
074ed1e to
810d900
Compare
|
Updated again to work even harder to try to make most of the CI configurations capable of running this test. |
|
Hm, that's interesting, apparently many headers have |
|
I don't understand the smartos failure. Did they accidentally release a bad header for mmap (missing MAP_FILE) in v14, or is there something wrong with that CI container? |
|
It looks like |
|
Yeah, it's just weird that it's only missing on v15. The tests appear to have passed on smartos v14 and v16. Can I have CI run again, please? |
5d2dbed to
cf0d1a2
Compare
On Apple, you'd currently get EINVAL. On Linux, this already worked (it just truncates the indivisual writes like I do in this PR). On Windows, you'd currently get an undocumented internal NT error code 0x800705aa (I think it essentially maps to ENOMEM). So if we wanted to make this consistently an error, I'd probably go with making it EINVAL everywhere. In the PR, we've instead essentially gone with doing the Linux behavior (of splitting the writes). partially working CI: https://ci.nodejs.org/job/libuv-test-commit/1697/ |
bnoordhuis
left a comment
There was a problem hiding this comment.
I only did a partial review because the ABI breakage needs to be worked out first.
| .. warning:: It is discouraged to set `len` to a large value as that may | ||
| result in spurious failures. Specifically, Windows may fail on | ||
| writes larger than about 511 MB, and various Unicies may fail | ||
| on IO larger than about 2 GB. Instead it is generally better to |
There was a problem hiding this comment.
| on IO larger than about 2 GB. Instead it is generally better to | |
| on I/O larger than about 2 GB. Instead it is generally better to |
| #define UV_STREAM_FIELDS \ | ||
| /* number of bytes queued for writing */ \ | ||
| size_t write_queue_size; \ | ||
| uint64_t write_queue_size; \ |
There was a problem hiding this comment.
This breaks ABI on 32-bits architectures, as does changing the return type of uv_stream_get_write_queue_size().
Yeah, as the bugfix keeps snowballing, I'm starting to feel like this can't entirely land in v1.x at all. Instead, I'm thinking we should just return an error code when you exceed the ability for libuv to represent the operation correctly or the OS to process it (not breaking because it's already broken, it'd just be more consistently handled). Then in v2 we can revisit this and consider whether more APIs should use 64-bit numbers. |
|
That sounds like an acceptable way forward to me. To recap: for v1.x that means returning UV_EINVAL when > 2 GB on Unices and > 511 MB (?) on Windows? |
Change the type of `Buffer::kMaxLength` to size_t because upcoming changes in V8 will allow typed arrays > 2 GB on 64 bits platforms. Not all platforms handle file reads and writes > 2 GB though so keep enforcing the 2 GB typed array limit for I/O operations. Fixes: nodejs#31399 Refs: libuv/libuv#1501
Change the type of `Buffer::kMaxLength` to size_t because upcoming changes in V8 will allow typed arrays > 2 GB on 64 bits platforms. Not all platforms handle file reads and writes > 2 GB though so keep enforcing the 2 GB typed array limit for I/O operations. Fixes: #31399 Refs: libuv/libuv#1501 PR-URL: #31406 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Change the type of `Buffer::kMaxLength` to size_t because upcoming changes in V8 will allow typed arrays > 2 GB on 64 bits platforms. Not all platforms handle file reads and writes > 2 GB though so keep enforcing the 2 GB typed array limit for I/O operations. Fixes: #31399 Refs: libuv/libuv#1501 PR-URL: #31406 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Shelley Vohr <codebytere@gmail.com>
|
ok, now we can support 64bit directly. |
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces libuv#1501
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces libuv#1501 Fixes libuv#3360
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces libuv#1501 Fixes libuv#3360
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces libuv#1501 Fixes libuv#3360
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces libuv#1501 Fixes libuv#3360
Because libuv truncates the result of every call to INT32_MAX, it needs to internally limit operations to INT32_MAX to be safe to use libuv. This isn't an API change, since these operations weren't guaranteed to work, and in fact usually failed in bizare ways already. This is very long in coming, since we've had a lot of compiler warnings about this and several PRs to fix this open for a decade, but the main consumers that usually fix things didn't care (nodejs is 32-bit and julia patched this downstream more than a decade ago, though it did run into this again recently by mistake with sendfile). Replaces #1501 Fixes #3360
On some platforms, notably OSX, read/readv/write/writev all return
an EINVAL if the requested number of bytes is greater than INT32_MAX.
We therefore limit each such call to 2GB on all platforms. On
platforms where this is not an issue, the overhead of an extra syscall
for every 2GB of read/write is negligible.
(For original PR and issue report, see JuliaLang#33)
Fixed link: JuliaAttic/libuv-archive#33