buffer: runtime-deprecate Buffer ctor by default

[seishun]

Creating a new PR as suggested in #7152 (comment).

Some backstory:

• In Node.js 6.0.0, new Buffer APIs (Buffer.from(), Buffer.allocUnsafe() and Buffer.alloc()) were introduced to address security concerns of the Buffer constructor, which was deprecated in the docs in the same version. (see buffer: add Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe(), soft-deprecate Buffer(num) #4682, Buffer(number) is unsafe #4660)
• In Node.js 8.0.0, a "pending deprecation" for the Buffer constructor was introduced. When Node.js is launched with the --pending-deprecation flag, the use of Buffer() and new Buffer() causes a DeprecationWarning to be emitted. (see src, buffer: add --pending-deprecation flag #11968)
• It was proposed to revisit this and possibly move the warning from behind the flag in Node.js 9.0.0. (see buffer: discuss future direction of Buffer constructor API #9531 (comment), buffer: discuss future direction of Buffer constructor API #9531 (comment)).

Several reasons for enabling runtime deprecation by default, in no particular order:

• Potentially prevents possible DoS vulnerabilities caused by accidentally calling Buffer(num) instead of Buffer(string) due to insufficient input validation.
• Buffer(num) zero-fills by default since 8.0.0, preventing accidental data disclosure. (see buffer: zero fill Buffer(num) by default #12141) However, it was not backported to earlier Node.js versions, which could result in security issues if new code is written with the assumption of zero-filling and then run on Node.js versions that don't zero-fill. (this and above is described in more detail here)
• Some developers might be unaware that Buffer(num) zero-fills and still be using it in performance-critical code, where Buffer.allocUnsafe() might be more appropriate.
• Calling Buffer() without new would be runtime-deprecated as well, so this functionality could potentially be removed in a later Node.js version. This would make it possible to refactor Buffer into a proper ES6 class that can be subclassed.
• Following "pending deprecation" with actual runtime deprecation will create a precedent that will cause more people to use the --pending-deprecation flag to test their code in the future. This will in turn allow us to introduce new runtime deprecations more smoothly by going through the "behind-the-flag" stage first.
• Eventually, most code will use the new Buffer API, which will make it less likely that people new to Node.js will ever need to learn about the deprecated API.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

buffer

[jasnell]

@ChALkeR ... Would it be possible to do another ecosystem scan on the current state of Buffer use?

[refack]

Would it be possible to do another ecosystem scan on the current state of Buffer use?

I run a query on a Gzemnid db from 02-2017, it overflowed, but I estimate that there are ~10KLoCs in several 100s of packages using new Buffer(

[jasnell]

I think we're still not far enough along in the adoption of the new Buffer methods to pull this off, at least not in time for 9.0.0. My recommendation would be to let this sit for a while longer.

Ping @nodejs/tsc

[seishun]

@jasnell As far as I can tell from the deprecation policy, there are no conditions for runtime deprecation related to the adoption of alternative APIs. It only says Documentation-Only Deprecations may land in a Node.js minor release but must not be upgraded to a Runtime Deprecation until the next major release and 9.0.0 will be the third major release since the documentation-only deprecation landed.

If in your opinion these conditions are insufficient, then perhaps the deprecation policy should be updated to precisely define the adoption requirements that must be fulfilled before runtime deprecation can land. Otherwise I'm afraid runtime deprecation of the Buffer constructor (and likely other things in the future) will remain a constantly moving target.

[jasnell]

It's not so much a limitation of the policy. That is, there's nothing in the policy that says we can't move to a runtime deprecation right now, I'm just not convinced that it's a good idea yet. Also, don't get me wrong, I want to have a runtime deprecation here.

[mcollina]

I am -1 to land this without hard data that the community has moved to the new API.
IMHO the amount of breakage is not worth it. I consider the buffer problem as solved.

[seishun]

@jasnell

That is, there's nothing in the policy that says we can't move to a runtime deprecation right now, I'm just not convinced that it's a good idea yet.

That's my concern, deciding when it's a good idea on ad-hoc basis might lead to inconsistent handling of deprecations and frustration for everyone involved.

@mcollina

IMHO the amount of breakage is not worth it.

Could you clarify what you mean by breakage and how you evaluate its amount?

[trevnorris]

To clarify, this would only be on the v9.x branch and not be backported to v8.x?

[mcollina]

@ChALkeR did some checks in the past, as far as I can remember. I expect this to still be highly used. I am still adding safe-buffer to multiple modules every month, and the process is definitely not finished.

Again, I've never been in favor and there is very little evidence to change my mind. I'd be very happy if this happens anyway, but it would require a TSC vote.

I might be ok to land this in Node 10, then all supported Node.js lines would have had the new API from the x.0.0 release.

[jasnell]

@trevnorris

To clarify, this would only be on the v9.x branch and not be backported to v8.x

That is correct. This would be a semver-major change.

[refack]

I'm trying to compile a full report (LOCs & Package numbers with new Buffer() vs. those with Buffer.*)

[Trott]

Would it make sense to do a CITGM run on this PR? I don't expect the results to change anyone's mind, but perhaps a clean CITGM run would be A Good Start anyway to keep conversation focused on things we can observe and measure.

[refack]

Would it make sense to do a CITGM run on this PR?

I'll trigger a run, at the end of the day.

[refack]

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/984/

[seishun]

@mcollina

I might be ok to land this in Node 10, then all supported Node.js lines would have had the new API from the x.0.0 release.

All supported Node.js lines (4.x, 6.x and 8.x) already have the new API, see 7090481.

Node 10 will be LTS, which probably means more people will install it, and therefore more people will see the warning if it lands in 10.0.0 compared to 9.0.0 (unless the usage of Buffer() drops significantly in 6 months).

[mcollina]

All supported Node.js lines (4.x, 6.x and 8.x) already have the new API,

Node 4.3.2 is still fairly popular inside lambda, and it does not have that API.
It was introduced in Node v4.5.0.

If Buffer() continue to be used, then no, I'm 👎 on this change. However, you can proactively go and fix the ecosystem. I have been doing that extensively, but we are not there yet. If 10 is too early, then it might be 11 or 12.

[seishun]

Node 4.3.2 is still fairly popular inside lambda

It's unsupported, so how is it relevant to your statement regarding supported Node.js lines?

However, you can proactively go and fix the ecosystem.

Thanks for your suggestion. Unfortunately, many module maintainers aren't eager to proactively move to new APIs, see for example browserify/browserify#1647 and izy521/discord.io#72. (Which I can totally understand, which brings us back to my point about --pending-deprecation. As it is now, people don't see any reason to believe the API will be runtime deprecated any time soon, so they don't see any point in code churn.)

[mcollina]

The way most companies work with Node, is that they do not think "node 4 LTS" is a moving target, but in fact they think of it as a group of releases.

Node 4.3.2 is a point-in-time release that does not have this feature, and it never will because it's released. So, an author cannot say that my module XYZ supports Node 4 if they use the new methods, but rather Node 4.5+.

Saying to leading module authors "we do not care about your opinion and we will force you to update" is a message that I cannot support. Convince them to migrate to the new API, and then make the switch.

[trevnorris]

The fact that Buffer is basically a shortcut to alloc() and from() doesn't make me feel like going full deprecation is urgent, or possibly necessary. Are there any security risks to still using it (except for passing in a number when the user thought it was a string, without an encoding)?

IMO the first thing that should happen is to update the docs and remove all the new from new Buffer(). It's useless. Actually, random shot but possibly worth while, what about only fully deprecating the usage of new Buffer() first?

[jasnell]

/me thinks out loud .... Eventually I'd really like to see us move away from Buffer entirely in favor of Uint8Array, while keeping the ability to create non-initialized Uint8Array instances... but that's just crazy talk.

[seishun]

Are there any security risks to still using it (except for passing in a number when the user thought it was a string, without an encoding)?

AFAIK none other than the two I listed in OP. But we don't runtime deprecate only due to security risks, do we?

Actually, random shot but possibly worth while, what about only fully deprecating the usage of new Buffer() first?

I thought you were interested in turning Buffer into an ES6 class. Encouraging dropping new would be counterproductive to that.

[BridgeAR]

So how do we progress here? It seems like there are still quite a few -1.

I am relatively certain that this is still used quite a lot in the wild but I also do not think that a lot of people care much. Convincing everyone will be very hard and not possible out of my perspective.

I personally think it might be a good idea to print out the warning in case numbers are passed to the constructor as that is the main pain point and far less people use it like this. So this might be a compromise between fully deprecating the constructor now or later?

I guess this is mainly a decision for the @nodejs/tsc

[Trott]

I personally think it might be a good idea to print out the warning in case numbers are passed to the constructor as that is the main pain point and far less people use it like this. So this might be a compromise between fully deprecating the constructor now or later?

A PR to do that along with CITGM results might be useful. It would be interesting (and possibly useful in pushing things forward) to compare those CITGM results with the CITGM results of a full runtime deprecation.

(Of course, all that might also be a lot of work that convinces nobody of anything. So, you know, just don't do it if you're going to be resentful if that turns out to be the result.)

[seishun]

@Trott
If you think there's a chance it will move things forward, then sure: #15608. (took less than an hour)

[BridgeAR]

@seishun this is definitely something that should land at some point but I doubt that this is going to happen in the next six month. Do you want to keep this open and get the opinion of the TSC about it or would it be fine to close this?

[ChALkeR]

Also, even if the package gets fixed, other packages relying on unfixed versions is also a giant problem out there. Ignoring node_modules would not help.

[ChALkeR]

We should notify end users that there is problematic code in their deps, and full deprecation is currently the only way how we can do that.

I have ideas how to improve that in the future (automatic ecosystem code/deps analysis, automatic notifications, and stuff), but that won't be ready soon, and the Buffer issue needs to get fixed.

The said analysis/notifications are now done in semi-manual way (basically — I file PRs), and that will also help to minimize the negative effects.

[seishun]

@mcollina

This is still extremely popular, and the main reason it received so much push back from the ecosystem was because it created a lot of burden to maintainers.

Could you clarify how being more explicit on how to fix this problem would decrease the burden to maintainers? Do you think there are many maintainers who don't know how to fix this problem?

[ChALkeR]

@seishun I have been filing the PRs, and this is indeed non-trivial in some cases.
I think that including a manual in the docs won't hurt. I can help writing that in the code part, but it would be better if someone more fluent in English than me worded things.

[mcollina]

Could you clarify how being more explicit on how to fix this problem would decrease the burden to maintainers? Do you think there are many maintainers who don't know how to fix this problem?

Let's make an example: module A is using Buffer(string), but the developer is using module B, which depends on A (B -> A). Very likely, the end user will open an issue on B, not A. This only creates maintenance burden for B's maintainer.

[seishun]

Very likely, the end user will open an issue on B, not A.

Which is correct, since B's maintainer would need to bump A's version anyway. Also, A could be unmaintained.

this is indeed non-trivial in some cases

Could you provide some examples?

[fhinkel]

I'm adding tsc-agenda to this so we can talk about it at our meeting this week.

The TSC has approved runtime-deprecating Buffer constructor in 10.0.0

@Trott, since the TSC has approved runtime-deprecating Buffer constructor, does this still need to be on tomorrow's agenda? IMHO the issue tracker is a much better place to discuss implementation details than a meeting.

[Trott]

@fhinkel This appears to have stalled. If @mcollina can respond to @seishun's last comment, we can remove it from the agenda. Otherwise, time is short to get this landed with enough time to back out if it turns out to have more far-reaching negative impact than anticipated. So I'd prefer it stay on the agenda if it's not making any progress.

Also, we probably want to review #19079 at the meeting if there isn't movement on the items in that list and/or no conversation in that issue.

[mcollina]

Very likely, the end user will open an issue on B, not A.

Which is correct, since B's maintainer would need to bump A's version anyway. Also, A could be unmaintained.

This is not correct, as fixing this by using safe-buffer is not semver-major change, and it would not require any action from B's author. This is what caused the massive community uproar the last time. With the current error message, B author would received a lot of heat, but he will not be able to do anything about it because the problem is in A. A author should receive the pressure to update, and ideally backport the fix to old release lines. Having issues opened on B would only increase the frustration of B's author.
This is the type of issues that I'm referring to: davidmarkclements/0x#52.

I keep my -1 on landing this without a deprecation message change, we need to point to a guide that explains to our users how to fix this. I'm happy if this goes to a placeholder document for now, and the guide is filled in a follow-up PR.

If you want to progress anyway, we can vote on this on the next @nodejs/tsc meeting.

---

@Trott the question for @seishun was to a comment @ChALkeR made #15346 (comment).

[seishun]

This is not correct, as fixing this by using safe-buffer is not semver-major change, and it would not require any action from B's author.

That wouldn't work if B depends on an exact, or a very old, version of A, or if A is unmaintained.

Personally, I don't see any issues with informing B's author about an issue in a dependency either way. They may choose to wait until A is fixed, or they may choose to just drop A if it happens to be a trivial change.

I keep my -1 on landing this without a deprecation message change, we need to point to a guide that explains to our users how to fix this.

Even if we agree on what the guide should say, this is something that would apply to all deprecation warnings. I suggest discussing it in a separate issue, rather than singling out Buffer constructor.

[mcollina]

Personally, I don't see any issues with informing B's author about an issue in a dependency either way. They may choose to wait until A is fixed, or they may choose to just drop A if it happens to be a trivial change.

We got several tweets and emails because of this in the past. It is very annoying if you are maintaining some popular packages.

Even if we agree on what the guide should say, this is something that would apply to all deprecation warnings. I suggest discussing it in a separate issue, rather than singling out Buffer constructor.

Very likely we should, but in all the other runtime deprecations we have never deprecated a massively used API. It's must on this one.

[seishun]

I suppose in that case the TSC should decide whether the Buffer warning needs special treatment, or we need to come up with a generic guide to be provided with all deprecation warnings.

[BridgeAR]

@seishun should this stay open since #19524 landed?

[seishun]

Not sure. I could either update this PR to do the deprecation warning everywhere or just make a new one. I'm fine either way.

[BridgeAR]

@seishun I believe it is probably best to open a new one as this is pretty much the oldest open PR and old PRs do not get as much attention (even though this one probably does not lack any attention ;-) ).

I guess opening a new one could actually very soon target 11.x.

[ryzokuken]

@seishun what's the status on this? Is this abandoned or are you still working on it?

[seishun]

I would like to know whether I should rebase this PR or create a new one.

@nodejs/collaborators what do you think?

👍 rebase
😆 new one

[ryzokuken]

@seishun ow, the dreadful mention. If I were you, I'd probably choose by myself. Any of the two methods work and both have their merits.

[seishun]

Sorry for the mention and thanks for the input. Closing to create a new PR.

[ryzokuken]

Awesome! Looking forward to it.