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

Warning on Buffer(var) and new Buffer(var) #693

Closed
mcollina opened this Issue Nov 22, 2016 · 29 comments

Comments

6 participants
@mcollina
Copy link

commented Nov 22, 2016

I think standard should emit a warning for Buffer(myvar) and new Buffer(myvar).

IMHO safe things are:

  • Buffer(42) and new Buffer(42)
  • Buffer(myvar, 'utf8') and new Buffer(myvar, 'utf8')
  • Buffer('hello') and new Buffer('hello')

All of those can run on any version of node, and provide an additional layer of security even if we do not zero fill, etc.

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2016

Yeah sounds good!

On Tue, Nov 22, 2016 at 6:47 PM Matteo Collina notifications@github.com
wrote:

I think standard should emit a warning for Buffer(myvar) and new
Buffer(myvar).

IMHO safe things are:

  • Buffer(42) and new Buffer(42)
  • Buffer(myvar, 'utf8') and new Buffer(myvar, 'utf8')
  • Buffer('hello') and new Buffer('hello')

All of those can run on any version of node, and provide an additional
layer of security even if we do not zero fill, etc.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#693, or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWleo2kz3XvDxQasKI19oGhYy10meAyks5rAyrCgaJpZM4K5vVn
.

@dcousens

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Why is new Buffer(myvar) unsafe, that is, if myvar is a number.
I agree otherwise.

This would break a lot of code since I often use dynamic buffer sizes...

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

if myvar is undefined bad things could happen

On Wed, Nov 23, 2016 at 2:33 AM Daniel Cousens notifications@github.com
wrote:

Why is new Buffer(myvar) unsafe, that is, if myvar is a number.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#693 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWlepcsIppntFAnHl57Lv9BNlMq0SgFks5rA5ffgaJpZM4K5vVn
.

@LinusU

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

One thing we could do is warn when it's being used without new since that currently prints warnings in Node 7...

@dcousens

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

@LinusU babel already does that 👍 , but I see no problem with us doing it too.

@LinusU

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

I would find it very nice since I'm not using babel :)

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

I like this idea. It would push people in the right direction without Node core needing to print a deprecation warning in the console.

I found an eslint plugin rule no-deprecated-api that disallows Buffer() constructor usage entirely: It's also prevents usage of other long-deprecated APIs, like domain, util.isArray, etc., which is probably nice to have as well.

@mcollina What do you think about just requiring Buffer.from(), etc.? It's a tad bit aggressive, but the API is in fact deprecated in the docs. We could release this as semver major.

@mafintosh

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

If this lands, standard should encourage people to use the safe-buffer module.
Also it would be cool if new Buffer(myvar) was allowed if myvar was statically analized to be 100% a number. That way code like new Buffer(BUFFER_SIZE) would work which is nice (bunch of browserify transforms already do this).

@mafintosh

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2016

Also this is a way better solution that deprecating anything in Node core IMO \o/

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

If this lands, standard should encourage people to use the safe-buffer module.

Yeah! The error message already encourages that!

@mcollina

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

@mcollina What do you think about just requiring Buffer.from(), etc.? It's a tad bit aggressive, but the API is in fact deprecated in the docs. We could release this as semver major.

It really depends. I'm in favor of not adding one more npm dep to support old nodes, but I am ok with suggesting that where new Buffer(myvar) is used.

I do not think 'disable all deprecated things' is good, I think we should just target each deprecation individually. Domains have no other equivalent, and there are good uses for them.

Buffer is special, we would probably need to put out a web page on how to deal with the error anyway (also the one currently in core).

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

I'm in favor of not adding one more npm dep to support old nodes, but I am ok with suggesting that where new Buffer(myvar) is used.

Currently we don't do "warnings". Just "errors" to keep things simple. Users could decide to ignore warnings, and that creates permanent noise in CI if/when they're ignored. So it's all-or-nothing.

I do not think 'disable all deprecated things' is good, I think we should just target each deprecation individually. Domains have no other equivalent, and there are good uses for them.

Yeah, this is reasonable. However, I'd argue that almost no one should use domains. If someone really needs domains, they can disable the rule with a comment: /* eslint-disable node/no-deprecated-api */ I think it's probably still a net win to discourage deprecated APIs, even if some advanced users bump into the restriction.

Also, if we make it semver major, then this would only apply to new code.

@mcollina

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

I'm in favor of not adding one more npm dep to support old nodes, but I am ok with suggesting that where new Buffer(myvar) is used.

Currently we don't do "warnings". Just "errors" to keep things simple. Users could decide to ignore warnings, and that creates permanent noise in CI if/when they're ignored. So it's all-or-nothing.

My bad, I am too wired in the other discussion. I meant errors.

I do not think 'disable all deprecated things' is good, I think we should just target each deprecation individually. Domains have no other equivalent, and there are good uses for them.

Yeah, this is reasonable. However, I'd argue that almost no one should use domains. If someone really needs domains, they can disable the rule with a comment: /* eslint-disable node/no-deprecated-api */ I think it's probably still a net win to discourage deprecated APIs, even if some advanced users bump into the restriction.

Agreed. The point is: do we want to automatically discourage any use of things deprecated in core? IMHO those errors are all semver-major changes, and it's better to include them individually. But, let's error on domains.

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

@mcollina Yeah, that's a good question. Given core's trend towards breaking changes, I can see them deprecating something else that is widely used, and then this rule would inherit it automatically, even for people on older (supported) versions of Node, like Node v4.

Related: mysticatea/eslint-plugin-node#58

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

Just ran the test suite with this rule enabled, and found this:

# tests 422
# pass  350
# fail  72

Almost all the errors were because of the Buffer API. There were also a few others, which are reasonable:

  • 'os.tmpDir' was deprecated since v7. Use 'os.tmpdir()' instead.
  • 'fs.exists' was deprecated since v4. Use 'fs.stat()' or 'fs.access()' instead.
@mcollina

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

Good. Can you link the full list of offenders on a gist?

I still think we should be in control of what we error on.

@feross

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

The ability to ignore individual rules was merged into eslint-plugin-node, so I think we're a go on this feature.

@feross feross added this to the standard v9 milestone Feb 8, 2017

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

I'd be OK with just disallowing Buffer/new Buffer entirely, in favour of: Buffer.from, Buffer.alloc and Buffer.allocUnsafe.

So many explosions will happen though.
I have a lot of code that relies on new Buffer(var) that is still supporting Node <4.5.0

@feross

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

@dcousens Yes, I agree. I plan to put this into the v10 beta. It's a new major version, so upgrading will be opt-in and users can wait to do it if they don't want to fix their code right away.

One of the things that's nice about having standard warn about these deprecations is that they're not user-facing like when Node.js core adds deprecations, which means users won't open up issues saying "your module is using deprecated methods, please fix".

We want to be careful to only warn about deprecations when the replacement code works in Node LTS versions (4 and 6). Otherwise, we'll force users to write code that only works in Node 6, for example, and that goes much too far.

Update: this has moved from v9 to the v10 milestone.

@feross feross modified the milestones: standard v9, standard v10 Feb 9, 2017

@feross

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

tl;dr: This will be part of the standard v10 beta.

The ecosystem impact with this change is extreme (18%).

# tests 287
# pass  235
# fail  52

Despite the large breakage, I believe this is the right thing to do. Tools like standard are a much better place to warn users about Node deprecations than inside Node itself, in my humble opinion.

standard should do its part to get users to stop using deprecated APIs so hopefully in a few years Node will finally be able to remove these old APIs.

One thing to temper this data... lots of my own modules appear in the tests and I use Buffer a lot. The Buffer deprecation was the source of most of the errors, so this might be overstating the ecosystem impact.

After fixing all my own modules, the new results are better, at 13%.

# tests 287
# pass  250
# fail  37

@feross feross closed this Mar 2, 2017

feross added a commit to standard/eslint-config-standard that referenced this issue Mar 2, 2017

@dcousens

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

I would have a lot of breakage... but, for me, its breakage I want.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

@dcousens I would have a lot of breakage... but, for me, its breakage I want.

Yep, I've actually been meaning to switch all my modules over to the new buffer methods for a while, but this new rule finally provided the impetus.

By the way, when Node <4.5.0 support is desired, safe-buffer is a good option.

@dcousens

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

for me, its breakage I want.

I take this back; the amount of breakage has been insane... I've had to lock so many packages down, from standard: * to standard: ^9.0.0 (too many changes otherwise).

@mafintosh

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2017

Yea, been locking down deps as well for the same reason.

@feross

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

@dcousens @mafintosh I totally get it, I did that on a few packages too. I plan to eventually get them onto standard 10 when I have time.

I think Node.js is still determined to hard-deprecate buffer (by printing a warning) eventually so I still think it's the right thing to warn maintainers about this before we all get tons of issues from confused users who are using a new Node.js version. Lmk if you disagree.

@mcollina

This comment has been minimized.

Copy link
Author

commented Apr 29, 2017

@feross I think adding a little utility that fixes it automatically for strings might be handy. I fixed 300+ instances with a couple of vim macros.

@dcousens

This comment has been minimized.

Copy link
Member

commented Apr 29, 2017

@mcollina the changes are easy... the maintenance of breakage down the chain since you are effectively enforcing node >=4.5.0... that is where the headaches begin.

@mcollina

This comment has been minimized.

Copy link
Author

commented Apr 29, 2017

@dcousens with safe-buffer, I applied those changes easily enough. It's just one line of code more.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.