Address unpack race conditions using path reservations

isaacs · isaacs · commit b2a97e15615b · 2021-08-09T13:41:02.000-07:00
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.
diff --git a/lib/unpack.js b/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')
@@ -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')
@@ -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
@@ -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
