From 570b40f9ca4ac08e7c6b45d08a5b906f4d03a681 Mon Sep 17 00:00:00 2001 From: "David Humphrey (:humph) david.humphrey@senecacollege.ca" Date: Fri, 17 Oct 2014 12:55:36 -0400 Subject: [PATCH 1/3] Add safety around accessing node for an OFD, clean-up xattr code for path vs. ofd --- src/filesystem/implementation.js | 127 ++++++++++++++++--------------- src/open-file-description.js | 27 ++++++- 2 files changed, 90 insertions(+), 64 deletions(-) diff --git a/src/filesystem/implementation.js b/src/filesystem/implementation.js index 3e7e09b..14c7d13 100644 --- a/src/filesystem/implementation.js +++ b/src/filesystem/implementation.js @@ -269,45 +269,26 @@ function find_node(context, path, callback) { /** * set extended attribute (refactor) */ -function set_extended_attribute (context, path_or_fd, name, value, flag, callback) { - var path; - - function set_xattr (error, node) { - var xattr = (node ? node.xattrs[name] : null); - - function update_time(error) { - if(error) { - callback(error); - } else { - update_node_times(context, path, node, { ctime: Date.now() }, callback); - } - } - - if (error) { +function set_extended_attribute (context, path, node, name, value, flag, callback) { + function update_time(error) { + if(error) { callback(error); - } - else if (flag === XATTR_CREATE && node.xattrs.hasOwnProperty(name)) { - callback(new Errors.EEXIST('attribute already exists', path_or_fd)); - } - else if (flag === XATTR_REPLACE && !node.xattrs.hasOwnProperty(name)) { - callback(new Errors.ENOATTR(null, path_or_fd)); - } - else { - node.xattrs[name] = value; - context.putObject(node.id, node, update_time); + } else { + update_node_times(context, path, node, { ctime: Date.now() }, callback); } } - if (typeof path_or_fd == 'string') { - path = path_or_fd; - find_node(context, path_or_fd, set_xattr); + var xattrs = node.xattrs; + + if (flag === XATTR_CREATE && xattrs.hasOwnProperty(name)) { + callback(new Errors.EEXIST('attribute already exists', path)); } - else if (typeof path_or_fd == 'object' && typeof path_or_fd.id == 'string') { - path = path_or_fd.path; - context.getObject(path_or_fd.id, set_xattr); + else if (flag === XATTR_REPLACE && !xattrs.hasOwnProperty(name)) { + callback(new Errors.ENOATTR(null, path)); } else { - callback(new Errors.EINVAL('path or file descriptor of wrong type', path_or_fd)); + xattrs[name] = value; + context.putObject(node.id, node, update_time); } } @@ -848,7 +829,7 @@ function stat_file(context, path, callback) { } function fstat_file(context, ofd, callback) { - context.getObject(ofd.id, callback); + ofd.getNode(context, callback); } function lstat_file(context, path, callback) { @@ -1315,7 +1296,7 @@ function ftruncate_file(context, ofd, length, callback) { if(length < 0) { callback(new Errors.EINVAL('length cannot be negative')); } else { - context.getObject(ofd.id, read_file_data); + ofd.getNode(context, read_file_data); } } @@ -1358,13 +1339,20 @@ function futimes_file(context, ofd, atime, mtime, callback) { callback(new Errors.EINVAL('atime and mtime must be positive integers')); } else { - context.getObject(ofd.id, update_times); + ofd.getNode(context, update_times); } } function setxattr_file(context, path, name, value, flag, callback) { path = normalize(path); + function setxattr(error, node) { + if(error) { + return callback(error); + } + set_extended_attribute(context, path, node, name, value, flag, callback); + } + if (typeof name != 'string') { callback(new Errors.EINVAL('attribute name must be a string', path)); } @@ -1376,12 +1364,19 @@ function setxattr_file(context, path, name, value, flag, callback) { callback(new Errors.EINVAL('invalid flag, must be null, XATTR_CREATE or XATTR_REPLACE', path)); } else { - set_extended_attribute(context, path, name, value, flag, callback); + find_node(context, path, setxattr); } } function fsetxattr_file (context, ofd, name, value, flag, callback) { - if (typeof name != 'string') { + function setxattr(error, node) { + if(error) { + return callback(error); + } + set_extended_attribute(context, ofd.path, node, name, value, flag, callback); + } + + if (typeof name !== 'string') { callback(new Errors.EINVAL('attribute name must be a string')); } else if (!name) { @@ -1392,7 +1387,7 @@ function fsetxattr_file (context, ofd, name, value, flag, callback) { callback(new Errors.EINVAL('invalid flag, must be null, XATTR_CREATE or XATTR_REPLACE')); } else { - set_extended_attribute(context, ofd, name, value, flag, callback); + ofd.getNode(context, setxattr); } } @@ -1400,16 +1395,17 @@ function getxattr_file (context, path, name, callback) { path = normalize(path); function get_xattr(error, node) { - var xattr = (node ? node.xattrs[name] : null); - - if (error) { - callback (error); + if(error) { + return callback(error); } - else if (!node.xattrs.hasOwnProperty(name)) { + + var xattrs = node.xattrs; + + if (!xattrs.hasOwnProperty(name)) { callback(new Errors.ENOATTR(null, path)); } else { - callback(null, node.xattrs[name]); + callback(null, xattrs[name]); } } @@ -1427,16 +1423,17 @@ function getxattr_file (context, path, name, callback) { function fgetxattr_file (context, ofd, name, callback) { function get_xattr (error, node) { - var xattr = (node ? node.xattrs[name] : null); - if (error) { - callback(error); + return callback(error); } - else if (!node.xattrs.hasOwnProperty(name)) { + + var xattrs = node.xattrs; + + if (!xattrs.hasOwnProperty(name)) { callback(new Errors.ENOATTR()); } else { - callback(null, node.xattrs[name]); + callback(null, xattrs[name]); } } @@ -1447,7 +1444,7 @@ function fgetxattr_file (context, ofd, name, callback) { callback(new Errors.EINVAL('attribute name cannot be an empty string')); } else { - context.getObject(ofd.id, get_xattr); + ofd.getNode(context, get_xattr); } } @@ -1455,7 +1452,9 @@ function removexattr_file (context, path, name, callback) { path = normalize(path); function remove_xattr (error, node) { - var xattr = (node ? node.xattrs : null); + if (error) { + return callback(error); + } function update_time(error) { if(error) { @@ -1465,19 +1464,18 @@ function removexattr_file (context, path, name, callback) { } } - if (error) { - callback(error); - } - else if (!xattr.hasOwnProperty(name)) { + var xattrs = node.xattrs; + + if (!xattrs.hasOwnProperty(name)) { callback(new Errors.ENOATTR(null, path)); } else { - delete node.xattrs[name]; + delete xattrs[name]; context.putObject(node.id, node, update_time); } } - if (typeof name != 'string') { + if (typeof name !== 'string') { callback(new Errors.EINVAL('attribute name must be a string', path)); } else if (!name) { @@ -1491,6 +1489,10 @@ function removexattr_file (context, path, name, callback) { function fremovexattr_file (context, ofd, name, callback) { function remove_xattr (error, node) { + if (error) { + return callback(error); + } + function update_time(error) { if(error) { callback(error); @@ -1499,14 +1501,13 @@ function fremovexattr_file (context, ofd, name, callback) { } } - if (error) { - callback(error); - } - else if (!node.xattrs.hasOwnProperty(name)) { + var xattrs = node.xattrs; + + if (!xattrs.hasOwnProperty(name)) { callback(new Errors.ENOATTR()); } else { - delete node.xattrs[name]; + delete xattrs[name]; context.putObject(node.id, node, update_time); } } @@ -1518,7 +1519,7 @@ function fremovexattr_file (context, ofd, name, callback) { callback(new Errors.EINVAL('attribute name cannot be an empty string')); } else { - context.getObject(ofd.id, remove_xattr); + ofd.getNode(context, remove_xattr); } } diff --git a/src/open-file-description.js b/src/open-file-description.js index e401822..d703488 100644 --- a/src/open-file-description.js +++ b/src/open-file-description.js @@ -1,6 +1,31 @@ -module.exports = function OpenFileDescription(path, id, flags, position) { +var Errors = require('./errors.js'); + +function OpenFileDescription(path, id, flags, position) { this.path = path; this.id = id; this.flags = flags; this.position = position; +} + +// Tries to find the node associated with an ofd's `id`. +// If not found, an error is returned on the callback. +OpenFileDescription.prototype.getNode = function(context, callback) { + var id = this.id; + var path = this.path; + + function check_if_node_exists(error, node) { + if(error) { + return callback(error); + } + + if(!node) { + return callback(new Errors.EBADF('file descriptor refers to unknown node', path)); + } + + callback(null, node); + } + + context.getObject(id, check_if_node_exists); }; + +module.exports = OpenFileDescription; From 1a4be5e2fdc6037c0c2f01ad7ac4211fd944ae65 Mon Sep 17 00:00:00 2001 From: "David Humphrey (:humph) david.humphrey@senecacollege.ca" Date: Fri, 17 Oct 2014 13:16:09 -0400 Subject: [PATCH 2/3] Add test for deleted node accessed via ofd --- tests/spec/fs.open.spec.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/spec/fs.open.spec.js b/tests/spec/fs.open.spec.js index 9b6498a..5e4911e 100644 --- a/tests/spec/fs.open.spec.js +++ b/tests/spec/fs.open.spec.js @@ -106,4 +106,31 @@ describe('fs.open', function() { }); }); }); + + it('should error if an ofd\'s node goes away while open', function(done) { + var fs = util.fs(); + + fs.writeFile('/myfile', 'data', function(error) { + if(error) throw error; + + fs.open('/myfile', 'r', function(error, fd) { + if(error) throw error; + + // Delete the file while it's still open + fs.unlink('/myfile', function(error) { + if(error) throw error; + + // This should fail now, since fd points to a bad node + fs.fstat(fd, function(error, result) { + expect(error).to.exist; + expect(error.code).to.equal('EBADF'); + expect(result).not.to.exist; + + fs.close(fd); + done(); + }); + }); + }); + }); + }); }); From f9e21cd576ad551e8dd914b1a9f7e843e84110a3 Mon Sep 17 00:00:00 2001 From: "David Humphrey (:humph) david.humphrey@senecacollege.ca" Date: Fri, 17 Oct 2014 13:34:29 -0400 Subject: [PATCH 3/3] Add note about https://github.com/filerjs/filer/issues/314 --- tests/spec/fs.open.spec.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/spec/fs.open.spec.js b/tests/spec/fs.open.spec.js index 5e4911e..203faa4 100644 --- a/tests/spec/fs.open.spec.js +++ b/tests/spec/fs.open.spec.js @@ -107,6 +107,10 @@ describe('fs.open', function() { }); }); + /** + * This test is currently correct per our code, but incorrect according to the spec. + * When we fix https://github.com/filerjs/filer/issues/314 we'll have to update this. + */ it('should error if an ofd\'s node goes away while open', function(done) { var fs = util.fs();