Skip to content

Quality: collectStream callback invoked twice on error-prone streams#854

Open
kumburovicbranko682-boop wants to merge 1 commit into
archiverjs:masterfrom
kumburovicbranko682-boop:contribai/improve/quality/collectstream-callback-invoked-twice-on-
Open

Quality: collectStream callback invoked twice on error-prone streams#854
kumburovicbranko682-boop wants to merge 1 commit into
archiverjs:masterfrom
kumburovicbranko682-boop:contribai/improve/quality/collectstream-callback-invoked-twice-on-

Conversation

@kumburovicbranko682-boop

Copy link
Copy Markdown

✨ Code Quality

Problem

The collectStream function registers separate listeners for 'error' and 'end' events that both call the same callback, with no guard preventing double invocation. While standard Node.js streams should not emit 'end' after 'error', this library processes arbitrary external streams (user-supplied), and non-standard stream implementations can violate this contract. If the callback is called twice (e.g., first with an error, then with data), downstream code may attempt to use a destroyed resource, call res.send() after headers are sent, or corrupt archive state — causing production crashes that are difficult to diagnose.

Severity: medium
File: lib/utils.js

Solution

Add a done flag to ensure the callback is only invoked once:

export function collectStream(source, callback) {
var collection = [];
var size = 0;
var done = false;

source.on("error", function (err) {
if (done) return;
done = true;
callback(err);
});

source.on("data", function (chunk) {
collection.push(chunk);
size += chunk.length;
});

source.on("end", function () {
if (done) return;
done = true;

var buf = Buffer.alloc(size);
var offset = 0;

collection.forEach(function (data) {
  data.copy(buf, offset);
  offset += data.length;
});

callback(null, buf);

});
}

Changes

  • lib/utils.js (modified)

Testing

  • Existing tests pass
  • Manual review completed
  • No new warnings/errors introduced


🤖 About this PR

This pull request was generated by ContribAI, an AI agent
that helps improve open source projects. The change was:

  1. Discovered by automated code analysis
  2. Generated by AI with context-aware code generation
  3. Self-reviewed by AI quality checks

If you have questions or feedback about this PR, please comment below.
We appreciate your time reviewing this contribution!

Closes #853

The collectStream function registers separate listeners for 'error' and 'end' events that both call the same callback, with no guard preventing double invocation. While standard Node.js streams should not emit 'end' after 'error', this library processes arbitrary external streams (user-supplied), and non-standard stream implementations can violate this contract. If the callback is called twice (e.g., first with an error, then with data), downstream code may attempt to use a destroyed resource, call res.send() after headers are sent, or corrupt archive state — causing production crashes that are difficult to diagnose.


Affected files: utils.js

Signed-off-by: kumburovicbranko682-boop <295886834+kumburovicbranko682-boop@users.noreply.github.com>

@gameroman gameroman left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: collectstream callback invoked twice on error-prone streams

2 participants