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

buffer: runtime-deprecate Buffer ctor by default #15346

Closed
wants to merge 1 commit into from
Closed

buffer: runtime-deprecate Buffer ctor by default #15346

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Sep 11, 2017

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

Some backstory:

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

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Sep 11, 2017
@jasnell
Copy link
Member

jasnell commented Sep 12, 2017

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

@refack
Copy link
Contributor

refack commented Sep 12, 2017

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
Copy link
Member

jasnell commented Sep 14, 2017

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

@jasnell jasnell requested a review from a team September 14, 2017 19:43
@seishun
Copy link
Contributor Author

seishun commented Sep 14, 2017

@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
Copy link
Member

jasnell commented Sep 14, 2017

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
Copy link
Member

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
Copy link
Contributor Author

seishun commented Sep 14, 2017

@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
Copy link
Contributor

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

@mcollina
Copy link
Member

@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 jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 14, 2017
@jasnell
Copy link
Member

jasnell commented Sep 14, 2017

@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
Copy link
Contributor

refack commented Sep 14, 2017

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

@Trott
Copy link
Member

Trott commented Sep 14, 2017

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
Copy link
Contributor

refack commented Sep 14, 2017

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

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

@refack
Copy link
Contributor

refack commented Sep 15, 2017

@seishun
Copy link
Contributor Author

seishun commented Sep 15, 2017

@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
Copy link
Member

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
Copy link
Contributor Author

seishun commented Sep 15, 2017

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

jasnell commented Sep 15, 2017

/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
Copy link
Contributor Author

seishun commented Sep 16, 2017

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
Copy link
Member

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
Copy link
Member

Trott commented Sep 25, 2017

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
Copy link
Contributor Author

seishun commented Sep 25, 2017

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

@BridgeAR
Copy link
Member

BridgeAR commented Oct 1, 2017

@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
Copy link
Member

ChALkeR commented Mar 1, 2018

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
Copy link
Member

ChALkeR commented Mar 1, 2018

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
Copy link
Contributor Author

seishun commented Mar 1, 2018

@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
Copy link
Member

ChALkeR commented Mar 1, 2018

@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
Copy link
Member

mcollina commented Mar 1, 2018

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
Copy link
Contributor Author

seishun commented Mar 1, 2018

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?

@Trott Trott added this to the 10.0.0 milestone Mar 1, 2018
@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 2, 2018
@fhinkel
Copy link
Member

fhinkel commented Mar 6, 2018

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.

@jasnell jasnell removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 6, 2018
@Trott
Copy link
Member

Trott commented Mar 6, 2018

@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
Copy link
Member

mcollina commented Mar 7, 2018

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
Copy link
Contributor Author

seishun commented Mar 7, 2018

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
Copy link
Member

mcollina commented Mar 7, 2018

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
Copy link
Contributor Author

seishun commented Mar 7, 2018

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
Copy link
Member

@seishun should this stay open since #19524 landed?

@seishun
Copy link
Contributor Author

seishun commented Apr 10, 2018

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
Copy link
Member

@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.

@jasnell jasnell modified the milestones: 10.0.0, 11.0.0 Apr 19, 2018
@ryzokuken
Copy link
Contributor

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

@seishun
Copy link
Contributor Author

seishun commented Jun 10, 2018

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
Copy link
Contributor

@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
Copy link
Contributor Author

seishun commented Jun 15, 2018

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

@seishun seishun closed this Jun 15, 2018
@ryzokuken
Copy link
Contributor

Awesome! Looking forward to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.