Node: stream.Readable unpipes the wrong stream when piped into multiple streams

[niels4]

• Version: 6.8.0 and later
• Platform: all
• Subsystem: stream.Readable

Since node 6.8.0 there is a bug where unpiping a stream from a readable stream that has a _readableState.pipesCount > 1 will cause it to remove the first stream in the _.readableState.pipes array no matter where in the list the dest stream was.

Example test case:

"use strict"
const PassThrough = require('stream').PassThrough

const source = PassThrough()
const dest1 = PassThrough()
const dest2 = PassThrough()

source.pipe(dest1)
source.pipe(dest2)

source.unpipe(dest2)

console.log(source._readableState.pipes === dest1) //false
console.log(source._readableState.pipes === dest2) //true

As you can see, the wrong stream was unpiped

It looks like this is the commit that broke things (https://github.com/nodejs/node/commit/2e568d95bdd689494b163f1cfe8bbc38f32e45ed#diff-ba6a0df0f5212f5cba5ca5179e209a17R670)
The variable used to splice was renamed to index on line 670, however the splice call on line 674 is still using the incorrect variable i.

[addaleax]

@niels4 Wow, that’s a pretty big bug to go unnoticed until now. Since you basically have the test case ready and identified what the bug is, do you want to go ahead with a pull request? The CONTRIBUTING.md has some pointers but feel free to ask anything here or in #node-dev on Freenode if anything’s unclear.

[niels4]

No problem, thanks for moving so quickly on this

[lpinca]

Kinda sad that all reviewers, me included, missed that. Nice find @niels4.

[addaleax]

/cc @nodejs/streams fyi

[MylesBorins]

/cc @nodejs/release @nodejs/lts if we can get a fix in ASAP I would like to put out a v6.8.2 tomorrow

[addaleax]

Yeah I’ll have one ready if @niels4 doesn’t open a PR. In that case I’d be happy to hear a name + email address @niels4 for attribution?

[gibfahn]

@TheAlphaNerd Do you mean v6.9.1 rather than v6.8.2?

[MylesBorins]

yes I did 😊

On Wed, Oct 19, 2016, 1:21 PM Gibson Fahnestock [email protected]
wrote:

@TheAlphaNerd https://github.com/TheAlphaNerd Do you mean v6.9.1 rather
than v6.8.2?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/9170#issuecomment-254796232, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV9nQSLB6mp7cBUBTu5EyJE0p4BnKks5q1gtWgaJpZM4KaMCF
.