Skip to content

Commit

Permalink
Address unpack race conditions using path reservations
Browse files Browse the repository at this point in the history
This addresses a race condition where one archive entry depends on an
earlier archive entry completing its unpack operation.

For example, a File entry followed by a Link to that file would fail if
the File was not written before the link() call was attempted, raising
an ENOENT.

Or, a File entry at a/b could be clobbered mid-write by an entry at
a/b/c.

This was never a problem for npm packages, because those tarballs are
created as a point-in-time snapshot of the package file tree.  Indeed,
even in the generic case, it's a bit of an edge case.  However, this
race condition led to some flaky tests, and could certainly be a problem
in general tar archive usage, especially if someone were to use
tar.update() often to append entries to an archive.

Address unpack race conditions using path reservations

This addresses a race condition where one archive entry depends on an
earlier archive entry completing its unpack operation.

For example, a File entry followed by a Link to that file would fail if
the File was not written before the link() call was attempted, raising
an ENOENT.

Or, a File entry at a/b could be clobbered mid-write by an entry at
a/b/c.

This was never a problem for npm packages, because those tarballs are
created as a point-in-time snapshot of the package file tree.  Indeed,
even in the generic case, it's a bit of an edge case.  However, this
race condition led to some flaky tests, and could certainly be a problem
in general tar archive usage, especially if someone were to use
tar.update() often to append entries to an archive.
  • Loading branch information
isaacs committed Aug 9, 2021
1 parent f0fe3aa commit b2a97e1
Showing 1 changed file with 27 additions and 5 deletions.
32 changes: 27 additions & 5 deletions lib/unpack.js
@@ -1,5 +1,11 @@
'use strict'

// the PEND/UNPEND stuff tracks whether we're ready to emit end/close yet.
// but the path reservations are required to avoid race conditions where
// parallelized unpack ops may mess with one another, due to dependencies
// (like a Link depending on its target) or destructive operations (like
// clobbering an fs object to create one of a different type.)

const assert = require('assert')
const EE = require('events').EventEmitter
const Parser = require('./parse.js')
Expand All @@ -10,9 +16,12 @@ const mkdir = require('./mkdir.js')
const mkdirSync = mkdir.sync
const wc = require('./winchars.js')
const stripAbsolutePath = require('./strip-absolute-path.js')
const pathReservations = require('./path-reservations.js')
const normPath = require('./normalize-windows-path.js')

const ONENTRY = Symbol('onEntry')
const CHECKFS = Symbol('checkFs')
const CHECKFS2 = Symbol('checkFs2')
const ISREUSABLE = Symbol('isReusable')
const MAKEFS = Symbol('makeFs')
const FILE = Symbol('file')
Expand Down Expand Up @@ -92,6 +101,8 @@ class Unpack extends Parser {

super(opt)

this.reservations = pathReservations()

this.transform = typeof opt.transform === 'function' ? opt.transform : null

this.writable = true
Expand Down Expand Up @@ -428,20 +439,31 @@ class Unpack extends Parser {
}
}

const paths = [entry.path]
if (entry.linkpath)
paths.push(entry.linkpath)
this.reservations.reserve(paths, done => this[CHECKFS2](entry, done))
}
[CHECKFS2] (entry, done) {
entry.on('end', done)
this[MKDIR](path.dirname(entry.absolute), this.dmode, er => {
if (er)
if (er) {
done()
return this[ONERROR](er, entry)
}
fs.lstat(entry.absolute, (er, st) => {
if (st && (this.keep || this.newer && st.mtime > entry.mtime))
if (st && (this.keep || this.newer && st.mtime > entry.mtime)) {
this[SKIP](entry)
else if (er || this[ISREUSABLE](entry, st))
this[MAKEFS](null, entry)
done()
} else if (er || this[ISREUSABLE](entry, st))
this[MAKEFS](null, entry, done)
else if (st.isDirectory()) {
if (entry.type === 'Directory') {
if (!entry.mode || (st.mode & 0o7777) === entry.mode)
this[MAKEFS](null, entry)
else
fs.chmod(entry.absolute, entry.mode, er => this[MAKEFS](er, entry))
fs.chmod(entry.absolute, entry.mode,
er => this[MAKEFS](er, entry))
} else
fs.rmdir(entry.absolute, er => this[MAKEFS](er, entry))
} else
Expand Down

0 comments on commit b2a97e1

Please sign in to comment.