Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writable's instanceof implementation is faulty #14943

Closed
Li357 opened this issue Aug 19, 2017 · 1 comment
Closed

Writable's instanceof implementation is faulty #14943

Li357 opened this issue Aug 19, 2017 · 1 comment
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@Li357
Copy link

Li357 commented Aug 19, 2017

  • Version: Node v8.4.0
  • Platform: macOS
  • Subsystem: Stream

The implementation for Writable's @@hasInstance method is faulty. As described in this Stack Overflow post:

In this snippet, the statement f instanceof PipeWritable returns true (Node v8.4.0):

const stream = require('stream');
const fs = require('fs');

class PipeWritable extends stream.Writable {
    constructor () {
        super();
    }
}

const s = new PipeWritable();
const f = fs.createWriteStream('/tmp/test');

console.log(f instanceof PipeWritable); // true ... ???

Because of the hasInstance implementation, this line:

return object && object._writableState instanceof WritableState;

Allows for Writables to be subclasses of Writable. This is a side-effect of Duplex parasitically inheriting from Writable, making new Duplex() instanceof Writeable true but also making Writables instances of Writable subclasses. Maybe add another check to make sure only Duplexes can be instances of Writables, but not Writables instances of Writable subclasses?

[refack: replaced code copy with code reference]

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Aug 19, 2017
@addaleax
Copy link
Member

Can you try this diff?

diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
index 6e0eaf45b546..7de77958d56b 100644
--- a/lib/_stream_writable.js
+++ b/lib/_stream_writable.js
@@ -179,6 +179,8 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) {
     value: function(object) {
       if (realHasInstance.call(this, object))
         return true;
+      if (this !== Writable)
+        return false;
 
       return object && object._writableState instanceof WritableState;
     }

addaleax added a commit to addaleax/node that referenced this issue Aug 23, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: nodejs#14943
addaleax added a commit to addaleax/ayo that referenced this issue Aug 25, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: nodejs/node#14943
PR-URL: nodejs/node#14945
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax added a commit to ayojs/ayo that referenced this issue Aug 28, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: nodejs/node#14943
PR-URL: nodejs/node#14945
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 10, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 20, 2017
The current custom instanceof for `Writable` subclasses previously
returned false positives for instances of *other* subclasses of
`Writable` because it was inherited by these subclasses.

Fixes: #14943
PR-URL: #14945
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants