zlib: let a ZipFile read finish before close()

An entry read runs on a file descriptor shared with its ZipFile, but
reads did not take part in close()'s lifecycle: close() marked the
handle closed and released the fd while a read was still in flight, so
the read landed on a closed - or worse, an OS-reused - descriptor
(surfacing as EBADF, or a cross-file read once the number was
reclaimed), despite the class comment promising otherwise.

Track in-flight reads on the shared handle. close() now marks the handle
closing (rejecting new reads at once), waits for the in-flight reads to
finish on the still-open fd, and only then closes it; closeSync(), which
cannot wait, refuses while an asynchronous read is outstanding.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>

Author: pipobscure

lib/internal/zip/entry.js

@@ -175,6 +175,17 @@ function createEntryMeta(filename, options) {
   };
 }
 
+// Release one in-flight read on a shared ZipFile descriptor handle. When the
+// last read settles, wake a close() that is waiting to release the fd (see
+// ZipFile.close() in file.js), so a read never lands on a closed/reused fd.
+function endHandleRead(handle) {
+  if (--handle.reads === 0 && handle.drain !== null) {
+    const drain = handle.drain;
+    handle.drain = null;
+    drain();
+  }
+}
+
 /**
  * A single file or directory inside a ZIP archive: reading, writing, and
  * (de)serializing one archive member.
@@ -490,17 +501,30 @@ class ZipEntry {
   // The entry's raw compressed bytes as a bounded-memory chunk stream, read
   // straight from disk (file-backed entries only). Nothing is retained.
   async *#rawChunks() {
-    this.#liveDescriptor();
-    let pos = await this.#resolveContentOffset();
-    let remaining = this.compressedSize;
-    while (remaining > 0) {
-      const take = MathMin(READ_CHUNK_SIZE, remaining);
-      const chunk = Buffer.allocUnsafe(take);
-      // Re-check per chunk: the ZipFile may be closed mid-stream.
-      await readFdFully(this.#liveDescriptor(), chunk, pos);
-      pos += take;
-      remaining -= take;
-      yield chunk;
+    // Count the whole stream as one in-flight read so a concurrent close()
+    // waits for it (the finally runs when the consumer stops iterating); a
+    // stream begun after close() was requested is rejected up front.
+    const handle = this.#fd;
+    if (handle.closing) {
+      throw new ERR_INVALID_STATE(
+        'cannot read a ZipEntry after its backing ZipFile has been closed');
+    }
+    handle.reads++;
+    try {
+      this.#liveDescriptor();
+      let pos = await this.#resolveContentOffset();
+      let remaining = this.compressedSize;
+      while (remaining > 0) {
+        const take = MathMin(READ_CHUNK_SIZE, remaining);
+        const chunk = Buffer.allocUnsafe(take);
+        // Re-check per chunk: the ZipFile may be closed mid-stream.
+        await readFdFully(this.#liveDescriptor(), chunk, pos);
+        pos += take;
+        remaining -= take;
+        yield chunk;
+      }
+    } finally {
+      endHandleRead(handle);
     }
   }
   // Sync counterpart of #rawChunks().
@@ -533,18 +557,33 @@ class ZipEntry {
         `entry ${JSONStringify(this.name)} declares ${declared} bytes, ` +
         `exceeding the ${maxSize} byte limit`);
     }
-    const compressed = await this.#compressedBytes();
-    const data = await decodeMemberAsync(compressed, {
-      name: this.name,
-      flags: this.flags,
-      method: this.method,
-      crc32: this.crc32,
-      uncompressedSize: declared,
-    }, { verify: options?.verify, maxSize });
-    // `data === compressed` only on the store path; copy the in-memory case
-    // (the entry's retained buffer, see #compressedBytes()) so the result is
-    // caller-owned on every path.
-    return data === compressed && this.#fd === null ? Buffer.from(data) : data;
+    // Count this read on the shared handle so a concurrent close() waits for it
+    // rather than releasing the fd mid-read; a read begun after close() was
+    // requested is rejected up front (in-memory entries have no fd).
+    const handle = this.#fd;
+    if (handle !== null) {
+      if (handle.closing) {
+        throw new ERR_INVALID_STATE(
+          'cannot read a ZipEntry after its backing ZipFile has been closed');
+      }
+      handle.reads++;
+    }
+    try {
+      const compressed = await this.#compressedBytes();
+      const data = await decodeMemberAsync(compressed, {
+        name: this.name,
+        flags: this.flags,
+        method: this.method,
+        crc32: this.crc32,
+        uncompressedSize: declared,
+      }, { verify: options?.verify, maxSize });
+      // `data === compressed` only on the store path; copy the in-memory case
+      // (the entry's retained buffer, see #compressedBytes()) so the result is
+      // caller-owned on every path.
+      return data === compressed && this.#fd === null ? Buffer.from(data) : data;
+    } finally {
+      if (handle !== null) endHandleRead(handle);
+    }
   }
 
   /**

lib/internal/zip/file.js

@@ -19,6 +19,7 @@ const {
   MapPrototypeKeys,
   MapPrototypeSet,
   MathMin,
+  Promise,
   PromisePrototypeThen,
   PromiseResolve,
   SymbolAsyncDispose,
@@ -218,12 +219,14 @@ function checkMemberOverlap(members, centralDirectoryOffset) {
  * corrupt the archive.
  */
 class ZipFile {
-  // Shared descriptor handle ({ fd, closed }) handed to every ZipEntry this
-  // archive produces, so close() can invalidate them all at once. `#closing`
-  // is set synchronously the moment close()/closeSync() is called, gating any
-  // further public call; `handle.closed` is set once the fd is actually gone,
-  // gating reads through already-handed-out entries. Neither read ever falls
-  // through to a bare (possibly OS-reused) descriptor number.
+  // Shared descriptor handle ({ fd, closed, reads, drain }) handed to every
+  // ZipEntry this archive produces, so close() can invalidate them all at once.
+  // `#closing` is set synchronously the moment close()/closeSync() is called,
+  // gating any further public call; `handle.closed` then stops new reads, and
+  // `handle.reads` counts in-flight reads through already-handed-out entries so
+  // close() can wait for them to finish before releasing the fd. Neither a new
+  // nor an in-flight read ever falls through to a closed (possibly OS-reused)
+  // descriptor number.
   #handle;
   #closing = false;
   #closePromise = null;
@@ -240,7 +243,7 @@ class ZipFile {
    * @private
    */
   constructor(fd, centralHeaders, prefix, centralDirectoryOffset, comment, writable) {
-    this.#handle = { fd, closed: false };
+    this.#handle = { fd, closed: false, closing: false, reads: 0, drain: null };
     this.#writable = writable;
     this.#comment = comment;
     this.#centralDirectoryOffset = centralDirectoryOffset;
@@ -655,7 +658,16 @@ class ZipFile {
   close() {
     if (this.#closing) return this.#closePromise ?? PromiseResolve();
     this.#closing = true;
+    // Block *new* reads immediately; reads already in flight keep the still-open
+    // fd until they finish (handle.closed, set below, is what stops mid-read).
+    this.#handle.closing = true;
     this.#closePromise = this.#enqueue(async () => {
+      // Let reads already in flight through handed-out entries finish on the
+      // open fd before releasing it; endHandleRead() (entry.js) resolves this
+      // once the last one settles.
+      if (this.#handle.reads > 0) {
+        await new Promise((resolve) => { this.#handle.drain = resolve; });
+      }
       this.#handle.closed = true;
       MapPrototypeClear(this.#entries);
       await fsCloseAsync(this.#handle.fd);
@@ -669,7 +681,15 @@ class ZipFile {
   closeSync() {
     if (this.#closing) return;
     this.#assertNotBusy();
+    // A synchronous close cannot wait for an async read the way close() does,
+    // so refuse rather than pull the fd out from under one still in flight.
+    if (this.#handle.reads > 0) {
+      throw new ERR_INVALID_STATE(
+        'cannot synchronously close a ZipFile while an asynchronous read is ' +
+        'still in flight');
+    }
     this.#closing = true;
+    this.#handle.closing = true;
     this.#handle.closed = true;
     MapPrototypeClear(this.#entries);
     fs.closeSync(this.#handle.fd);