Permalink
Browse files

fs.WriteStream: Handle modifications to fs.open

If the fs.open method is modified via AOP-style extension, in between
the creation of an fs.WriteStream and the processing of its action
queue, then the test of whether or not the method === fs.open will fail,
because fs.open has been replaced.

The solution is to save a reference to fs.open on the stream itself when
the action is placed in the queue.

This fixes isaacs/node-graceful-fs#6.
  • Loading branch information...
1 parent 93eca95 commit 06ada03ed93d6f81caef24c6dade6e616fd843b1 @isaacs isaacs committed Apr 9, 2012
Showing with 56 additions and 3 deletions.
  1. +4 −3 lib/fs.js
  2. +52 −0 test/simple/test-fs-write-stream-change-open.js
View
@@ -1332,7 +1332,8 @@ var WriteStream = fs.WriteStream = function(path, options) {
this._queue = [];
if (this.fd === null) {
- this._queue.push([fs.open, this.path, this.flags, this.mode, undefined]);
+ this._open = fs.open;
+ this._queue.push([this._open, this.path, this.flags, this.mode, undefined]);
this.flush();
}
};
@@ -1374,7 +1375,7 @@ WriteStream.prototype.flush = function() {
cb(null, arguments[1]);
}
- } else if (method === fs.open) {
+ } else if (method === self._open) {
// save reference for file pointer
self.fd = arguments[1];
self.emit('open', self.fd);
@@ -1392,7 +1393,7 @@ WriteStream.prototype.flush = function() {
});
// Inject the file pointer
- if (method !== fs.open) {
+ if (method !== self._open) {
args.unshift(this.fd);
}
@@ -0,0 +1,52 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+
+var path = require('path'),
+ fs = require('fs');
+
+var file = path.join(common.tmpDir, 'write.txt');
+
+var stream = fs.WriteStream(file),
+ _fs_close = fs.close,
+ _fs_open = fs.open;
+
+// change the fs.open with an identical function after the WriteStream
+// has pushed it onto its internal action queue, but before it's
+// returned. This simulates AOP-style extension of the fs lib.
+fs.open = function() {
+ return _fs_open.apply(fs, arguments);
+};
+
+fs.close = function(fd) {
+ assert.ok(fd, 'fs.close must not be called with an undefined fd.');
+ fs.close = _fs_close;
+ fs.open = _fs_open;
+}
+
+stream.write('foo');
+stream.end();
+
+process.on('exit', function() {
+ assert.equal(fs.open, _fs_open);
+});

6 comments on commit 06ada03

@AndreasMadsen
Member

Soooo why do this not count as "protecting userland from doing stupid things" - if I'm not misstaken it is the nodecore convention not to protect userland.

An alternativ solution made in the userland module could be: copy all functions from require('fs') intro exports and extend the graceful-fs functions there need to be extended.

var fs = require('fs');
require('util')._extend(exports, fs);
exports.open = function () {
  return fs.open.apply(fs, arguments);
};
@bnoordhuis
Member

I saw the commit but I thought it was cleanup for domains, didn't notice it landed in v0.6. I agree with @AndreasMadsen, this doesn't seem like it belongs here.

@isaacs
isaacs commented on 06ada03 Apr 10, 2012

This is not protecting userland, it's protecting core from userland.

Relying on fs.open never being modified is not reasonable. There is no way to reliably know how many files have been opened without wrapping the fs.open method directly. Several libraries do this, and we do it in several of our own tests. If we're going to use that as some sort of magic flag, then we need to do so safely.

@bnoordhuis
Member

I don't know if I agree with that line of reasoning. This commit may protect against a particular kind of monkey patching but where do you draw the line? There must be hundreds of places in node.js where function overloading will break something somewhere. There is no reasonable defense against that safe for freezing the process object.

I think you should revert this patch, if only to stop people from complaining later on "yeah, but isaacs merged a patch that fixed his problem".

@isaacs
isaacs commented on 06ada03 Apr 11, 2012

The proper patch would be for us not to use fs.open as a marker object. Relying on object identity requires keeping a reference to the marker object. It's not just "we like monkey patching, so therefor we should make all monkey patching work." It's about being defensive with regard to reliance on object identity.

If something is an internal API, like the fs.WriteStream's _queue behavior, it should not be so brittle as to rely on a public property never being altered. This was fixing a bug, not adding a feature.

If there are other cases where we're relying on object identity, and not storing the marker internally, then those are other bugs. If they cause problems, and are easy to fix in a low-risk and low-complexity way, then we'll fix them.

@isaacs
isaacs commented on 06ada03 Apr 11, 2012

The proper patch would be for us not to use fs.open as a marker object.

By that, I mean, the push onto the fs.WriteStream queue should specify a string, or a private object, and use that as the marker, since then it would be immutable.

However, that change was more involved, and not as simple.

Please sign in to comment.