fs: allow int64 offset in fs.read/readSync/write/writeSync by zbjornson · Pull Request #26572 · nodejs/node

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 <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Ref #26563 PR-URL: #26572 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

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

Uh oh!

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

fs: allow int64 offset in fs.read/readSync/write/writeSync#26572
zbjornson wants to merge 4 commits intonodejs:masterfrom
zbjornson:zb/26563

zbjornson commented Mar 10, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

zbjornson commented Mar 14, 2019 •

edited

Loading

Uh oh!

ryzokuken left a comment

Uh oh!

Uh oh!

Uh oh!

zbjornson commented Mar 16, 2019 •

edited

Loading

Uh oh!

zbjornson commented Mar 18, 2019

Uh oh!

BridgeAR commented Mar 25, 2019

Uh oh!

Trott commented Aug 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

zbjornson commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

zbjornson commented Mar 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zbjornson commented Mar 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbjornson commented Mar 18, 2019

Uh oh!

BridgeAR commented Mar 25, 2019

Uh oh!

Trott commented Aug 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

zbjornson commented Mar 10, 2019 •

edited

Loading

zbjornson commented Mar 14, 2019 •

edited

Loading

zbjornson commented Mar 16, 2019 •

edited

Loading