unpack: keep path reservations longer

This avoids races when entries are zero-length, and emit end() before the fs object is fully finished and closed.
isaacs · Aug 9, 2021 · ea6f254 · ea6f254
1 parent b2a97e1
commit ea6f254
Showing 1 changed file with 39 additions and 27 deletions.
diff --git a/lib/unpack.js b/lib/unpack.js
@@ -45,6 +45,11 @@ const UID = Symbol('uid')
 const GID = Symbol('gid')
 const crypto = require('crypto')
 
+/* istanbul ignore next */
+const neverCalled = () => {
+  throw new Error('sync function called cb somehow?!?')
+}
+
 // Unlinks on Windows are not atomic.
 //
 // This means that if you have a file entry, followed by another
@@ -303,7 +308,7 @@ class Unpack extends Parser {
     return uint32(this.gid, entry.gid, this.processGid)
   }
 
-  [FILE] (entry) {
+  [FILE] (entry, fullyDone) {
     const mode = entry.mode & 0o7777 || this.fmode
     const stream = new fsm.WriteStream(entry.absolute, {
       mode: mode,
@@ -316,8 +321,12 @@ class Unpack extends Parser {
       if (er)
         return this[ONERROR](er, entry)
 
-      if (--actions === 0)
-        fs.close(stream.fd, _ => this[UNPEND]())
+      if (--actions === 0) {
+        fs.close(stream.fd, er => {
+          fullyDone()
+          er ? this[ONERROR](er, entry) : this[UNPEND]()
+        })
+      }
     }
 
     stream.on('finish', _ => {
@@ -356,15 +365,18 @@ class Unpack extends Parser {
     tx.pipe(stream)
   }
 
-  [DIRECTORY] (entry) {
+  [DIRECTORY] (entry, fullyDone) {
     const mode = entry.mode & 0o7777 || this.dmode
     this[MKDIR](entry.absolute, mode, er => {
-      if (er)
+      if (er) {
+        fullyDone()
         return this[ONERROR](er, entry)
+      }
 
       let actions = 1
       const done = _ => {
         if (--actions === 0) {
+          fullyDone()
           this[UNPEND]()
           entry.resume()
         }
@@ -389,12 +401,12 @@ class Unpack extends Parser {
     entry.resume()
   }
 
-  [SYMLINK] (entry) {
-    this[LINK](entry, entry.linkpath, 'symlink')
+  [SYMLINK] (entry, done) {
+    this[LINK](entry, entry.linkpath, 'symlink', done)
   }
 
-  [HARDLINK] (entry) {
-    this[LINK](entry, path.resolve(this.cwd, entry.linkpath), 'link')
+  [HARDLINK] (entry, done) {
+    this[LINK](entry, path.resolve(this.cwd, entry.linkpath), 'link', done)
   }
 
   [PEND] () {
@@ -445,7 +457,6 @@ class Unpack extends Parser {
     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) {
         done()
@@ -460,45 +471,46 @@ class Unpack extends Parser {
         else if (st.isDirectory()) {
           if (entry.type === 'Directory') {
             if (!entry.mode || (st.mode & 0o7777) === entry.mode)
-              this[MAKEFS](null, entry)
+              this[MAKEFS](null, entry, done)
             else
               fs.chmod(entry.absolute, entry.mode,
-                er => this[MAKEFS](er, entry))
+                er => this[MAKEFS](er, entry, done))
           } else
-            fs.rmdir(entry.absolute, er => this[MAKEFS](er, entry))
+            fs.rmdir(entry.absolute, er => this[MAKEFS](er, entry, done))
         } else
-          unlinkFile(entry.absolute, er => this[MAKEFS](er, entry))
+          unlinkFile(entry.absolute, er => this[MAKEFS](er, entry, done))
       })
     })
   }
 
-  [MAKEFS] (er, entry) {
+  [MAKEFS] (er, entry, done) {
     if (er)
       return this[ONERROR](er, entry)
 
     switch (entry.type) {
       case 'File':
       case 'OldFile':
       case 'ContiguousFile':
-        return this[FILE](entry)
+        return this[FILE](entry, done)
 
       case 'Link':
-        return this[HARDLINK](entry)
+        return this[HARDLINK](entry, done)
 
       case 'SymbolicLink':
-        return this[SYMLINK](entry)
+        return this[SYMLINK](entry, done)
 
       case 'Directory':
       case 'GNUDumpDir':
-        return this[DIRECTORY](entry)
+        return this[DIRECTORY](entry, done)
     }
   }
 
-  [LINK] (entry, linkpath, link) {
+  [LINK] (entry, linkpath, link, done) {
     // XXX: get the type ('file' or 'dir') for windows
     fs[link](linkpath, entry.absolute, er => {
       if (er)
         return this[ONERROR](er, entry)
+      done()
       this[UNPEND]()
       entry.resume()
     })
@@ -520,15 +532,15 @@ class UnpackSync extends Unpack {
       }
     }
 
-    const er = this[MKDIR](path.dirname(entry.absolute), this.dmode)
+    const er = this[MKDIR](path.dirname(entry.absolute), this.dmode, neverCalled)
     if (er)
       return this[ONERROR](er, entry)
     try {
       const st = fs.lstatSync(entry.absolute)
       if (this.keep || this.newer && st.mtime > entry.mtime)
         return this[SKIP](entry)
       else if (this[ISREUSABLE](entry, st))
-        return this[MAKEFS](null, entry)
+        return this[MAKEFS](null, entry, neverCalled)
       else {
         try {
           if (st.isDirectory()) {
@@ -539,17 +551,17 @@ class UnpackSync extends Unpack {
               fs.rmdirSync(entry.absolute)
           } else
             unlinkFileSync(entry.absolute)
-          return this[MAKEFS](null, entry)
+          return this[MAKEFS](null, entry, neverCalled)
         } catch (er) {
           return this[ONERROR](er, entry)
         }
       }
     } catch (er) {
-      return this[MAKEFS](null, entry)
+      return this[MAKEFS](null, entry, neverCalled)
     }
   }
 
-  [FILE] (entry) {
+  [FILE] (entry, _) {
     const mode = entry.mode & 0o7777 || this.fmode
 
     const oner = er => {
@@ -616,7 +628,7 @@ class UnpackSync extends Unpack {
     })
   }
 
-  [DIRECTORY] (entry) {
+  [DIRECTORY] (entry, _) {
     const mode = entry.mode & 0o7777 || this.dmode
     const er = this[MKDIR](entry.absolute, mode)
     if (er)
@@ -653,7 +665,7 @@ class UnpackSync extends Unpack {
     }
   }
 
-  [LINK] (entry, linkpath, link) {
+  [LINK] (entry, linkpath, link, _) {
     try {
       fs[link + 'Sync'](linkpath, entry.absolute)
       entry.resume()