Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Commit

Permalink
read: account for entries changed after _read
Browse files Browse the repository at this point in the history
If this.entries is changed after _read() has been called, we will be
out of sync and try to access an invalid index of this.entries. When
the entry cannot be found, we emit end and close, which can drop files
from reading.

(At least partially) fixes npm/npm#5082.

Credit: @evanlucas
Reviewed-By: @othiym23
PR-URL: #50
  • Loading branch information
evanlucas authored and othiym23 committed May 12, 2016
1 parent 3258080 commit a55ae72
Showing 1 changed file with 9 additions and 5 deletions.
14 changes: 9 additions & 5 deletions lib/dir-reader.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function DirReader (props) {
}

self.entries = null
self._entries = []
self._index = -1
self._paused = false
self._length = -1
Expand All @@ -47,6 +48,7 @@ DirReader.prototype._getEntries = function () {
if (er) return self.error(er)

self.entries = entries
self._entries = entries.slice()

This comment has been minimized.

Copy link
@jdalton

jdalton May 12, 2016

What's the distinction between _entries and entries? (I mean beyond than the shallow clone)

This comment has been minimized.

Copy link
@othiym23

othiym23 May 12, 2016

Contributor

That's it – it's a new private copy that won't get overwritten by fstream-ignore or fstream-npm (which inherit from fstream) mid-iteration.

This comment has been minimized.

Copy link
@evanlucas

evanlucas May 12, 2016

Author Contributor

self.entries is mutated by fstream-ignore and then the stream would end early because the lookup was by an index that was calculated on the array before mutation.

This comment has been minimized.

Copy link
@jdalton

jdalton May 12, 2016

Ah, so just a cache of entries to look up. Then entries is just a view of that. Cool, cool.

self.emit('entries', entries)
if (self._paused) self.once('resume', processEntries)
Expand Down Expand Up @@ -74,7 +76,7 @@ DirReader.prototype._read = function () {
}

self._index++
if (self._index >= self.entries.length) {
if (self._index >= self._entries.length) {
if (!self._ended) {
self._ended = true
self.emit('end')
Expand All @@ -83,12 +85,14 @@ DirReader.prototype._read = function () {
return
}

// ok, handle this one, then.

// save creating a proxy, by stat'ing the thing now.
var p = path.resolve(self._path, self.entries[self._index])
var nextEntry = self._entries[self._index]
if (!nextEntry) return this._read()

// ok, handle this one, then.
var p = path.resolve(self._path, nextEntry)
assert(p !== self._path)
assert(self.entries[self._index])
assert(nextEntry)

// set this to prevent trying to _read() again in the stat time.
self._currentEntry = p
Expand Down

0 comments on commit a55ae72

Please sign in to comment.