fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

zbjornson · 2019-03-10T19:28:40Z

Since v10.10.0, 'buf' can be any DataView, meaning the largest byteLength can be Float64Array.BYTES_PER_ELEMENT * kMaxLength = 17,179,869,176.

Fixes #26563

~~Eventually, the same change applies to fs.write*. If this current change is acceptable, I'll make the corresponding change there.~~ Done

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included - No. I can add a test but I don't see a way to do it without requiring a large amount of disk space and memory. benchmark N/A.
documentation is changed or added - No. If this is significant enough to note in the history section, let me know.
commit message follows commit guidelines

lib/fs.js

src/node_file.cc

zbjornson · 2019-03-14T04:43:35Z

Read for review again. I ended up changing directions a bit: offset can be 2⁵³-1, but len can only be uint32 because there's no point to supporting the full range:

uv_buf_init only takes unsigned int length.
On UNIX-y platforms, uv_buf_init can be simply changed to take size_t (uv_buf_t already has size_t len), but Linux read()/pread() will only transfer up to 2 GB - 4 KB per call.
On Windows, ReadFile can only transfer up to 4 GB.

User-space tiling with fs.read is easy and, with this patch allowing big offset values, possible; so I think this is a reasonable outcome.

(When I have time I can work on separate PRs for write and streams.)

ryzokuken

LG! PTAL at the comment.

src/node_file.cc

src/util-inl.h

zbjornson · 2019-03-16T19:50:19Z

Changed implementation a little bit so the IsSafeJsInt function mirrors ECMA262's Number.isSafeInteger spec.

zbjornson · 2019-03-18T20:07:32Z

Revised again to just widen the offset parameter, but leave length alone. Widening length needs a lot of changes because several functions in libuv and node_file.cc have int return types that overflow. Since the change would only benefit Windows but not Linux (idk about MacOS), I don't think it's worth the change.

BridgeAR · 2019-03-25T18:00:58Z

@zbjornson this requires a rebase due to recent changes. It would also be great if you could add a test that verifies that NaN will throw (it used to be coerced to 0).

PR-URL: #26572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Ref #26563 PR-URL: #26572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott · 2019-08-17T05:10:02Z

Landed in c3b8e50...5e3b4d6. 🎉

validateInteger() was renamed to validateSafeInteger() in nodejs#26572. However, this function also works with unsafe integers. This commit restores the old name, and adds some basic tests. PR-URL: nodejs#29184 Refs: nodejs#26572 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Notable changes: - **assert**: - do not repeat .throws() code (Ruben Bridgewater) [#28263](#28263) - wrap validation function errors (Ruben Bridgewater) [#28263](#28263) - fix generatedMessage property (Ruben Bridgewater) [#28263](#28263) - improve class instance errors (Ruben Bridgewater) [#28263](#28263) - **benchmark**: - use test/common/tmpdir consistently (João Reis) [#28858](#28858) - **build**: - make full-icu the default for releases (Richard Lau) [#29887](#29887) - update minimum Xcode version for macOS (Michael Dawson) [#29622](#29622) - **child_process**: - runtime deprecate \_channel (cjihrig) [#27949](#27949) - simplify spawn argument parsing (cjihrig) [#27854](#27854) - **console**: - display timeEnd with suitable time unit (Xavier Stouder) [#29251](#29251) - **deps**: - patch V8 to 7.8.279.14 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.12 (Myles Borins) [#29694](#29694) - patch V8 to 7.8.279.10 (Myles Borins) [#29694](#29694) - update V8's postmortem script (cjihrig) [#29694](#29694) - V8: cherry-pick 716875d (Myles Borins) [#29694](#29694) - update V8 to 7.8.279.9 (Myles Borins) [#29694](#29694) - V8: cherry-pick b33af60 (Michaël Zasso) [#28016](#28016) - update V8 to 7.6.303.28 (Michaël Zasso) [#28016](#28016) - **domain**: - error handler runs outside of its domain (Julien Gilli) [#26211](#26211) - **fs**: - make FSWatcher.start private (Lucas Holmquist) [#29905](#29905) - add runtime deprecate for file stream open() (Robert Nagy) [#29061](#29061) - allow int64 offset in fs.write/writeSync/fd.write (Zach Bjornson) [#26572](#26572) - use IsSafeJsInt instead of IsNumber for ftruncate (Zach Bjornson) [#26572](#26572) - allow int64 offset in fs.read/readSync/fd.read (Zach Bjornson) [#26572](#26572) - close file descriptor of promisified truncate (João Reis) [#28858](#28858) - **http**: - do not emit end after aborted (Robert Nagy) [#27984](#27984) - don't emit 'data' after 'error' (Robert Nagy) [#28711](#28711) - remove legacy parser (Anna Henningsen) [#29589](#29589) - throw if 'host' agent header is not a string value (Giorgos Ntemiris) [#29568](#29568) - replace superfluous connection property with getter/setter (Robert Nagy) [#29015](#29015) - fix test where aborted should not be emitted (Robert Nagy) [#20077](#20077) - remove default 'timeout' listener on upgrade (Luigi Pinca) [#26030](#26030) - **http, http2**: - remove default server timeout (Ali Ijaz Sheikh) [#27558](#27558) - **http2**: - remove security revert flags (Anna Henningsen) [#29141](#29141) - remove callback-based padding (Anna Henningsen) [#29144](#29144) - **lib**: - rename validateInteger to validateSafeInteger (Zach Bjornson) [#26572](#26572) - correct error.errno to always be numeric (Joyee Cheung) [#28140](#28140) - no need to strip BOM or shebang for scripts (Refael Ackermann) [#27375](#27375) - rework logic of stripping BOM+Shebang from commonjs (Gus Caplan) [#27768](#27768) - **module**: - runtime deprecate createRequireFromPath() (cjihrig) [#27951](#27951) - **readline**: - error on falsy values for callback (Sam Roberts) [#28109](#28109) - **repl**: - close file descriptor of history file (João Reis) [#28858](#28858) - **src**: - bring 425 status code name into accordance with RFC 8470 (Sergei Osipov) [#29880](#29880) - update NODE\_MODULE\_VERSION to 79 (Myles Borins) [#29694](#29694) - update NODE\_MODULE\_VERSION to 78 (Michaël Zasso) [#28918](#28918) - add error codes to errors thrown in C++ (Yaniv Friedensohn) [#27700](#27700) - use non-deprecated overload of V8::SetFlagsFromString (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 77 (Michaël Zasso) [#28016](#28016) - update NODE\_MODULE\_VERSION to 74 (Refael Ackermann) [#27375](#27375) - make process.env.TZ setter clear tz cache (Ben Noordhuis) [#20026](#20026) - enable V8's WASM trap handlers (Gus Caplan) [#27246](#27246) - **stream**: - throw unhandled error for readable with autoDestroy (Robert Nagy) [#29806](#29806) - always invoke callback before emitting error (Robert Nagy) [#29293](#29293) - invoke callback before emitting error always (Robert Nagy) [#29293](#29293) - do not flush destroyed writable (Robert Nagy) [#29028](#29028) - don't emit finish on error (Robert Nagy) [#28979](#28979) - disallow stream methods on finished stream (Robert Nagy) [#28687](#28687) - do not emit after 'error' (Robert Nagy) [#28708](#28708) - fix destroy() behavior (Robert Nagy) [#29058](#29058) - simplify `.pipe()` and `.unpipe()` in Readable (Weijia Wang) [#28583](#28583) - **tools**: - patch V8 to run on older XCode versions (Ujjwal Sharma) [#29694](#29694) - update V8 gypfiles (Michaël Zasso) [#29694](#29694) - support full-icu by default (Steven R. Loomis) [#29522](#29522) - **util**: validate formatWithOptions inspectOptions (Ruben Bridgewater) [#29824](#29824) PR-URL: #29504

Notable changes: * assert: * If the validation function passed to `assert.throws()` or `assert.rejects()` returns a value other than `true`, an assertion error will be thrown instead of the original error to highlight the programming mistake (Ruben Bridgewater). #28263 * If a constructor function is passed to validate the instance of errors thrown in `assert.throws()` or `assert.reject()`, an assertion error will be thrown instead of the original error (Ruben Bridgewater). #28263 * build: * Node.js releases are now built with default full-icu support. This means that all locales supported by ICU are now included and Intl-related APIs may return different values than before (Richard Lau). #29887 * The minimum Xcode version supported for macOS was increased to 10. It is still possible to build Node.js with Xcode 8 but this may no longer be the case in a future v13.x release (Michael Dawson). #29622 * child_process: * `ChildProcess._channel` (DEP0129) is now a Runtime deprecation (cjihrig). #27949 * console: * The output `console.timeEnd()` and `console.timeLog()` will now automatically select a suitable time unit instead of always using milliseconds (Xavier Stouder). #29251 * deps: * The V8 engine was updated to version 7.8. This includes performance improvements to object destructuring, memory usage and WebAssembly startup time (Myles Borins). #29694) * domain: * The domain's error handler is now executed with the active domain set to the domain's parent to prevent inner recursion (Julien Gilli). #26211 * fs: * The undocumented method `FSWatcher.prototype.start()` was removed (Lucas Holmquist). #29905 * Calling the `open()` method on a `ReadStream` or `WriteStream` now emits a runtime deprecation warning. The methods are supposed to be internal and should not be called by user code (Robert Nagy). #29061 * `fs.read/write`, `fs.readSync/writeSync` and `fd.read/write` now accept any safe integer as their `offset` parameter. The value of `offset` is also no longer coerced, so a valid type must be passed to the functions (Zach Bjornson). #26572 * http: * Aborted requests no longer emit the `end` or `error` events after `aborted` (Robert Nagy). #27984 #20077 * Data will no longer be emitted after a socket error (Robert Nagy). #28711 * The legacy HTTP parser (previously available under the `--http-parser=legacy` flag) was removed (Anna Henningsen). #29589 * The `host` option for HTTP requests is now validated to be a string value (Giorgos Ntemiris). #29568 * The `request.connection` and `response.connection` properties are now runtime deprecated. The equivalent `request.socket` and `response.socket` should be used instead (Robert Nagy). #29015 * http, http2: * The default server timeout was removed (Ali Ijaz Sheikh). #27558 * Brought 425 status code name into accordance with RFC 8470. The name changed from "Unordered Collection" to "Too Early" (Sergei Osipov). #29880 * lib: * The `error.errno` property will now always be a number. To get the string value, use `error.code` instead (Joyee Cheung). #28140 * module: * `module.createRequireFromPath()` is deprecated. Use `module.createRequire()` instead (cjihrig). #27951 * src: * Changing the value of `process.env.TZ` will now clear the tz cache. This affects the default time zone used by methods such as `Date.prototype.toString` (Ben Noordhuis). #20026 * stream: * The timing and behavior of streams was consolidated for a number of edge cases. Please look at the individual commits below for more information. PR-URL: #29504

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Mar 10, 2019

BridgeAR reviewed Mar 10, 2019

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

zbjornson force-pushed the zb/26563 branch 2 times, most recently from acbc60e to 0e7db78 Compare March 10, 2019 20:44

addaleax approved these changes Mar 10, 2019

View reviewed changes

BridgeAR approved these changes Mar 10, 2019

View reviewed changes

bnoordhuis reviewed Mar 11, 2019

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

zbjornson force-pushed the zb/26563 branch from 0e7db78 to ff731c5 Compare March 14, 2019 04:39

zbjornson changed the title ~~fs: allow int64 offset and length in fs.read/readSync~~ fs: allow int64 offset and uint32 length in fs.read/readSync Mar 14, 2019

ryzokuken approved these changes Mar 14, 2019

View reviewed changes

src/node_file.cc Outdated Show resolved Hide resolved

src/util-inl.h Outdated Show resolved Hide resolved

zbjornson force-pushed the zb/26563 branch 10 times, most recently from 8b28b6c to c5c1897 Compare March 16, 2019 18:56

zbjornson force-pushed the zb/26563 branch 4 times, most recently from cbf0760 to de27e44 Compare March 18, 2019 18:27

zbjornson changed the title ~~fs: allow int64 offset and uint32 length in fs.read/readSync~~ fs: allow int64 offset in fs.read/readSync Mar 18, 2019

Trott closed this Aug 17, 2019

zbjornson deleted the zb/26563 branch August 17, 2019 05:12

cjihrig mentioned this pull request Aug 17, 2019

lib: rename validateSafeInteger to validateInteger #29184

Merged

3 tasks

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

richardlau mentioned this pull request Jan 30, 2020

v12.15.0 release proposal #31368

Closed

joyeecheung mentioned this pull request Jun 8, 2020

src: retrieve binding data from the context #33139

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

zbjornson commented Mar 10, 2019 •

edited

Loading

zbjornson commented Mar 14, 2019 •

edited

Loading

ryzokuken left a comment

zbjornson commented Mar 16, 2019 •

edited

Loading

zbjornson commented Mar 18, 2019

BridgeAR commented Mar 25, 2019

Trott commented Aug 17, 2019

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

fs: allow int64 offset in fs.read/readSync/write/writeSync #26572

Conversation

zbjornson commented Mar 10, 2019 • edited Loading

Checklist

zbjornson commented Mar 14, 2019 • edited Loading

ryzokuken left a comment

Choose a reason for hiding this comment

zbjornson commented Mar 16, 2019 • edited Loading

zbjornson commented Mar 18, 2019

BridgeAR commented Mar 25, 2019

Trott commented Aug 17, 2019

zbjornson commented Mar 10, 2019 •

edited

Loading

zbjornson commented Mar 14, 2019 •

edited

Loading

zbjornson commented Mar 16, 2019 •

edited

Loading