fs: don't alter user provided `options` object #7831

thefourtheye · 2016-07-22T03:58:02Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

This patch makes a copy of the options object before altering it.

cc @nodejs/fs

thefourtheye · 2016-07-22T04:00:44Z

This is a fix for #7655. @micnic @thealphanerd This can be safely backported, if necessary.

micnic · 2016-07-22T08:27:58Z

LGTM with some comments

As I see, in some fs stream prototype constructors Object.create() is used instead of utils._extend(), it would be good to use a single object properties copying method

About tests, it would be good to have such tests for other methods that receive as parameters user defined objects, cc @nodejs/testing

cjihrig · 2016-07-22T13:26:03Z

test/parallel/test-fs-options-immutable.js

+);
+
+assert.doesNotThrow(() =>
+  fs.appendFile(1, 'ABCD', Object.freeze({flag: 'w'}), (e) => assert.ifError(e))


Please add common.mustCall() to the callback.

thefourtheye · 2016-07-22T18:23:19Z

@micnic @cjihrig Comments addressed. PTAL.

jasnell · 2016-07-26T14:02:14Z

lib/fs.js

@@ -1377,11 +1377,11 @@ fs.appendFileSync = function(path, data, options) {
    throwOptionsError(options);
  }

-  if (!options.flag)
-    options = util._extend({ flag: 'a' }, options);
+  // don't make changes directly on options object


minor nit... s/don't/Don't
(there are multiple comments in this doc that start with lower case that should be uppercase)

jasnell · 2016-07-26T14:05:42Z

LGTM

thefourtheye · 2016-07-27T04:31:02Z

cc @nodejs/collaborators

micnic · 2016-07-27T07:32:01Z

LGTM

jasnell · 2016-07-29T16:32:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/3456/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/41/

yorkie · 2016-07-30T09:08:13Z

Object.assign might be better than util._extend which would be deprecated by the former one day?

micnic · 2016-07-30T09:45:15Z

@yorkie from #7208 and #7255 it was decided to use the util._extend() as it has better performance than Object.assign()

ChALkeR · 2016-07-31T14:21:43Z

@thefourtheye Why does this remove Object.create() from tests? Does it break with Object.create()?

ChALkeR · 2016-07-31T14:26:17Z

> Object.create(Object.create({encoding: 'utf8'})).encoding
'utf8'
> util._extend({}, Object.create({encoding: 'utf8'})).encoding
undefined

Doesn't it make this semver-major?

yorkie · 2016-07-31T15:07:30Z

Why does this remove Object.create() from tests? Does it break with Object.create()?

@ChALkeR because the Object.create takes its arguments into prototype chain, not the direct field, but the Object.keys that the util._extend is internally using does not return keys from prototype chain as below:

The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

ChALkeR · 2016-07-31T15:13:13Z

@yorkie, so, this means that it actually broke those tests?

yorkie · 2016-07-31T15:15:21Z

so, this means that it actually broke those tests?

Yea, we have to change those cases IMO :-(

ChALkeR · 2016-07-31T17:01:52Z

Labeling as semver-major due to a breaking change. Note that it also changes existing tests expectations.

Feel free to remove the semver-major label if this will be changed so that all the existing tests pass.

Could we keep support for those somehow? Btw, this potentially could break some modules out there, so I'm not even sure if landing this in the current state to 7.0 without a deprecation cycle will be fine. Refs: #7912.

Note: it's not semver-major due to the actual intended change that is described in the commit title, it's semver-major because passing something like Object.create({encoding: 'utf8'}) worked before and doesn't work now.

jasnell · 2016-08-08T18:38:30Z

@nodejs/ctc ... any further thoughts on this?
@thefourtheye ... the PR needs to be rebased and updated.

silverwind · 2016-08-08T18:56:31Z

What is this options = Object.create(options) trying to achieve anyways?

thefourtheye · 2016-08-09T05:46:00Z

@jasnell I think I am blocked by #8006. I'll rebase and update the PR once I can test this locally.

thefourtheye · 2016-08-09T05:47:14Z

@silverwind We add few properties to options object passed by users, in few of the fs APIs. Object.create(options) would avoid mutating the original options object passed by user.

claudiorodriguez · 2016-08-09T08:26:06Z

@thefourtheye Wouldn't something like:

const options = { a: { b: 1 } };
const newOptions = Object.create(options);
newOptions.a.b = 2;

still alter the original options object?

mhdawson · 2016-10-05T21:09:07Z

The status post must be wrong as there was a failure on aix even though the check above show as green. I assume this was not seen earlier as the test did not make it that far.

not ok 338 parallel/test-fs-options-immutable
# events.js:160
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: ENOENT: no such file or directory, open '/home/iojs/node-tmp/tmp.0/streams'
  ---

mhdawson · 2016-10-05T21:12:44Z

Hmm, see it passed in an earlier CI run so not sure why that would fail unless there could be some conflict with temp directory generation and other tests running in parallel ?

Failure on PPC was unrelated as its related to the know test-tic-processor-issues covered in: #8725

mhdawson · 2016-10-05T21:22:35Z

Stress run on AIX https://ci.nodejs.org/job/node-stress-single-test/986/

mhdawson · 2016-10-05T22:12:59Z

Seems to fail consistently in stress run, did the test change since the successful CI run ?

mhdawson · 2016-10-06T15:29:19Z

Investigating locally, believe it may be because the test assumes the write will complete before the read starts which is not guaranteed

mhdawson · 2016-10-06T15:56:11Z

Yes believe it is a race condition in the test. This version passes reliably:

{
  const fileName = path.resolve(common.tmpDir, 'streams');
  assert.doesNotThrow(() =>
    fs.WriteStream(fileName, options).once('open', () => {
      assert.doesNotThrow(() => fs.ReadStream(fileName, options))
    })
  );
}

thefourtheye · 2016-10-09T07:01:36Z

Awesome. Thanks for digging deeper and providing a fix as well, @mhdawson :-)

https://ci.nodejs.org/job/node-stress-single-test/989/ is Green.

One last CI Run before landing: https://ci.nodejs.org/job/node-test-pull-request/4437/

This patch makes a copy of the `options` object before the fs module functions alter it. PR-URL: nodejs#7831 Fixes: nodejs#7655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nicu Micleușanu <micnic90@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

thefourtheye · 2016-10-09T10:19:35Z

I had to rebase because of the linter rule upgrade. New CI: https://ci.nodejs.org/job/node-test-pull-request/4440/

thefourtheye · 2016-10-09T14:01:51Z

FreeBSD failures are not related. Landing this now...

thefourtheye · 2016-10-09T14:09:31Z

Landed in 7542bdd

This patch makes a copy of the `options` object before the fs module functions alter it. PR-URL: #7831 Fixes: #7655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nicu Micleușanu <micnic90@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

jasnell · 2016-10-10T15:18:00Z

This does not land cleanly on v7.x-staging without the semver-major #7165 also applied.

This patch makes a copy of the `options` object before the fs module functions alter it. PR-URL: #7831 Fixes: #7655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nicu Micleușanu <micnic90@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

thefourtheye · 2016-10-10T16:13:27Z

@jasnell Oh, do you want me to PR this targeting 7.x?

jasnell · 2016-10-10T16:21:52Z

No no, sorry i wasn't clear. Both prs landed in v7.x-staging, I was just
documenting the dependency between them.

On Monday, October 10, 2016, Sakthipriyan Vairamani <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell Oh, do you want me to PR this
targeting 7.x?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7831 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eSvVQzKU4UWg9r8dMba0kGOutXNmks5qymQugaJpZM4JSa-n
.

Fishrock123 · 2016-10-10T17:25:28Z

Backport PRs will be necessary if desired, this appears to depend on #7165 / I don't know how to resolve easily.

This patch makes a copy of the `options` object before the fs module functions alter it. PR-URL: nodejs/node#7831 Fixes: nodejs/node#7655 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Nicu Micleușanu <micnic90@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>

thefourtheye added the fs Issues and PRs related to the fs subsystem / file system. label Jul 22, 2016

MylesBorins added the lts-watch-v4.x label Jul 22, 2016

thefourtheye mentioned this pull request Jul 22, 2016

fs: refactor "options" processing as a function #7165

Closed

2 tasks

cjihrig reviewed Jul 22, 2016
View reviewed changes

thefourtheye force-pushed the dont-alter-options branch from ef7ad0f to 54db1c6 Compare July 22, 2016 18:21

jasnell reviewed Jul 26, 2016
View reviewed changes

ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 31, 2016

thefourtheye removed the lts-watch-v4.x label Jul 31, 2016

thefourtheye force-pushed the dont-alter-options branch from 7be837a to 1ecce67 Compare October 9, 2016 06:30

thefourtheye force-pushed the dont-alter-options branch from 1ecce67 to e378a56 Compare October 9, 2016 10:15

thefourtheye closed this Oct 9, 2016

thefourtheye deleted the dont-alter-options branch October 9, 2016 14:09

Fishrock123 added dont-land-on-v4.x labels Oct 10, 2016

This was referenced Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

test: remove AIX guard in fs-options-immutable #12687

Closed

LiviaMedeiros mentioned this pull request May 27, 2022

test: add common.mustNotMutateObjectDeep() #43196

Merged

fs: don't alter user provided options object #7831

fs: don't alter user provided options object #7831

Conversation

thefourtheye commented Jul 22, 2016 • edited

Checklist

Affected core subsystem(s)

Description of change

thefourtheye commented Jul 22, 2016

micnic commented Jul 22, 2016

cjihrig Jul 22, 2016

Choose a reason for hiding this comment

thefourtheye commented Jul 22, 2016

jasnell Jul 26, 2016 • edited

Choose a reason for hiding this comment

jasnell commented Jul 26, 2016

thefourtheye commented Jul 27, 2016

micnic commented Jul 27, 2016

jasnell commented Jul 29, 2016 • edited

yorkie commented Jul 30, 2016

micnic commented Jul 30, 2016

ChALkeR commented Jul 31, 2016

ChALkeR commented Jul 31, 2016

yorkie commented Jul 31, 2016

ChALkeR commented Jul 31, 2016

yorkie commented Jul 31, 2016

ChALkeR commented Jul 31, 2016 • edited

jasnell commented Aug 8, 2016

silverwind commented Aug 8, 2016

thefourtheye commented Aug 9, 2016

thefourtheye commented Aug 9, 2016 • edited

claudiorodriguez commented Aug 9, 2016 • edited

mhdawson commented Oct 5, 2016 • edited

mhdawson commented Oct 5, 2016

mhdawson commented Oct 5, 2016 • edited

mhdawson commented Oct 5, 2016

mhdawson commented Oct 6, 2016

mhdawson commented Oct 6, 2016

thefourtheye commented Oct 9, 2016

thefourtheye commented Oct 9, 2016 • edited

thefourtheye commented Oct 9, 2016

thefourtheye commented Oct 9, 2016

jasnell commented Oct 10, 2016

thefourtheye commented Oct 10, 2016

jasnell commented Oct 10, 2016

Fishrock123 commented Oct 10, 2016

fs: don't alter user provided `options` object #7831

fs: don't alter user provided `options` object #7831

thefourtheye commented Jul 22, 2016 •

edited

jasnell Jul 26, 2016 •

edited

jasnell commented Jul 29, 2016 •

edited

ChALkeR commented Jul 31, 2016 •

edited

thefourtheye commented Aug 9, 2016 •

edited

claudiorodriguez commented Aug 9, 2016 •

edited

mhdawson commented Oct 5, 2016 •

edited

mhdawson commented Oct 5, 2016 •

edited

thefourtheye commented Oct 9, 2016 •

edited