unguarded fs.watchFile cache statWatchers checking fixed #1637

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

tshinnic commented Sep 3, 2011

fs.watchFile code had checks for cached previously seen filepaths where the
check was not adequately protected against property name collisions:

    if (statWatchers[filename]) {

could have false positives if the filepath was a chained Object property
name, e.g. 'hasOwnProperty'. Wild-looking errors would ensue:

TypeError: Object function hasOwnProperty() { [native code] } has no method 'addListener'
      at Object.watchFile (fs.js:646:8)
      at /node.js/node-1_fs/test/simple/test-fs-watch-file.js:101:8

Fix is to use hasOwnProperty in both places cache checks are done:

var inStatWatchers = Object.prototype.hasOwnProperty.bind(statWatchers);

  if (inStatWatchers(filename)) {

Change updates lib/fs.js and test/simple/test-fs-watch-file.js

tshinnic commented Sep 9, 2011

Updated with new commit after applying review comments. Description of commit notes the changes since review.

@tshinnic tshinnic unguarded fs.watchFile cache statWatchers checking fixed
Use hasOwnProperty to check filepath cache; previous code could fail if
a filepath duplicated a chained property name.
3ecd574

The (long) comments that were on the previous commit:

fs.watchFile code had checks for cached previously seen filepaths where the
check was not adequately protected against property name collisions:

  if (statWatchers[filename]) {

could have false positives if the filepath was a chained Object property
name, e.g. 'hasOwnProperty'. Wild-looking errors would ensue:

TypeError: Object function hasOwnProperty() { [native code] } has no method 'ad
      at Object.watchFile (fs.js:646:8)
      at /node.js/node-1_fs/test/simple/test-fs-watch-file.js:101:8

Fix is to use hasOwnProperty in both places cache checks are done:

function inStatWatchers(filename) {
  return Object.prototype.hasOwnProperty.call(statWatchers, filename);
}

  if (inStatWatchers(filename)) {

Changes updates lib/fs.js and test/simple/test-fs-watch-file.js

Changes since review of previous commit

A more conventional implementation of the hasOwnProperty check.

Removed extra console.logs

koichik closed this in 7dc2c49 Sep 12, 2011

koichik commented Sep 12, 2011

@tshinnic - Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment