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: discuss future direction of Buffer constructor API #9531

Closed
addaleax opened this Issue Nov 9, 2016 · 146 comments

Comments

Projects
None yet
@addaleax
Copy link
Member

addaleax commented Nov 9, 2016

In today’s CTC meeting we discussed reverting the DeprecationWarning for calling Buffer without new that was introduced in v7 (PR up here), and it became clear that we need to come up with a long-term plan on what exactly we want to achieve, how to do that and improve our messaging about it, both before and after our actions. I’ll try to sum up what exactly we are talking about; obviously, I am somewhat biased, having been involved in plenty of the previous discussion here. (This has still gotten pretty long btw, so I hope a lot of people will find the information in here useful enough to warrant a Wall of Text.)

The Buffer constructor has the usability flaw that it accepts input with different type signatures, so new Buffer('abcdef') and new Buffer(100) will both return valid buffers, and in the latter case, the Buffer will contain 100 bytes of unitialized memory. This is a security problem for two reasons:

  1. When passing unvalidated user input (e.g. from a JSON request) to the Buffer constructor where a string is expected but a number is actually passed, uninitialized memory will be returned:
// This is a dangerous example of converting a string to Base64!
new Buffer(someStringReceivedFromTheUser).toString('base64')

Passing the value 100 here will return a slice of memory that may contain garbage, but generally can contain any value previously stored in memory, including credentials, source code, and much more. @ChALkeR has a pretty good write-up of this: https://github.com/ChALkeR/notes/blob/master/Buffer-knows-everything.md

  1. Accidentally accepting large numeric values can very quickly increase resource usage, and can be turned into a Denial-of-Service attack against vulnerable applications.

Again, @ChALkeR has a very-good write-up on these security issues at https://github.com/ChALkeR/notes/blob/master/Lets-fix-Buffer-API.md. It predates the current Buffer.alloc()/Buffer.from() situation, but it contains a helpful FAQ with answers to questions like “Why not just make Buffer(number) zero-fill everything by default?”.

So far, in Node v6.0.0 the safer Buffer.alloc()/Buffer.from() API was introduced and later backported to the v5.10.0 and v4.5.0 releases. Additionally, v6.0.0 came with a documentation-only deprecation of the old Buffer() API.

In June, #7152 was opened, which seeks to deprecate the old Buffer() API using a runtime deprecation, i.e. printing a single warning per Node process when Buffer() or new Buffer() is executed for the first time. Currently, that PR is still open. A reduced version of it, #8169, was landed as a semver-major change in v7.0.0, that emits and displays DeprecationWarnings for uses of Buffer() only, but excludes uses of new Buffer().

I had summarized the goals and possible actions before that decision was made in #7152 (comment) ¹; And @jasnell has written a then-current long-term plan in #7152 (comment) that would include runtime deprecations of new Buffer() in v8.0.0 and later actual breaking changes to the Buffer constructor.

The reason for this distinction was trying to keep the possibility of making Buffer a proper ES6 class at some point in the far, far future open, which would mean that calling new Buffer() may always work. (Effects of turning Buffer into a class would be proper subclassability and breaking Buffer() without new. It is, however, completely possible to add a separate class to the API that would behave like the current Buffer implementation does, only with these differences.)

As a result of that deprecation for Buffer() without new in v7.0.0, significant pushback from well-known members of the community ensued, both in the threads on #7152 and #8169. On the one hand, it became clear that we failed in our messaging to make clear that the primary motivation for that change was helping our users avoid serious security issues; on the other hand, the added deprecation warning seemed to be incongruent with the expectations of stability and backwards compatibility that module authors and consumers have, as far as Node core is concerned.

As a result of this, the CTC decided to consider reverting the deprecation warning, possibly temporarily, and the corresponding PR is in #9529. The decision on that has yet to be made, but the desire has been expressed to reach a decision soon to limit the number of v7.x versions with possibly incongruent behaviour.

From following the discussions, it is obvious that the path forward is a contentious issue; right now, the opinions range from never introducing a runtime deprecation for any version of the Buffer constructor, to applying one for all uses of it at the next semver-major release in v8.0.0.

The strongest and most frequently expressed argument for fully runtime-deprecating the Buffer constructor soon remains that users may not be aware that parts of their application use an unsafe API and should be warned about that.

On the other side, the warning itself is perceived as a very disruptive change to the ecosystem, suggesting that it is definitely worth exploring alternative ways to reduce the usage of both Buffer() and new Buffer().

/cc @nodejs/collaborators


¹ It may or may not be obvious from the way I articulate my thoughts here – I try to stick to stating facts – but in hindsight, I regret writing it this way.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 9, 2016

More opinionated stuff from myself:
I do not think we should ever turn Buffer into an ES6 class; we can create a separate API for that and make Buffer a wrapper around that, as it has been discussed before, and nothing about Buffer() ever needs to be truly broken. I know my comment linked above mentions a possible breakage as a motivation for deprecating Buffer() earlier than new Buffer(), but I definitely don’t agree with my past self on this anymore. I do however see that there is significant security risk involved with their usage, and that that may warrant a full runtime deprecation at some point in the future.

Correspondingly, I would ask of others (although I am aware that I obviously can’t speak for everyone) to try and avoid giving “Subclassability” and “ES6 class” as reasons for the deprecations here; I think it is obvious that our messaging broke here, as reflected by the comments on #7152 and #8169. IMHO, this is primarily about security, and all other benefits of future changes pale in front of that.

@mafintosh

This comment has been minimized.

Copy link
Member

mafintosh commented Nov 10, 2016

@addaleax Thank you for the detailed write up. Much appreciated.

I'll suggest a way forward that have been mentioned by other people already.

Instead of deprecating anything start zero-filling buffers returned by the Buffer constructor, similar to Buffer.alloc. Back-port this change to old versions of node (I'd like to see this added all the way back to 0.10 but I know that isn't officially supported anymore).

A benefit of this approach is that old code will work just like before - no changes needed.

It also introduces an incentive for module authors to upgrade to the new API as the zero filling has a perceived performance penalty.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Nov 10, 2016

@mafintosh ... automatically zero-filling only addresses part of the issue. There are other aspects of the existing Buffer() use that are problematic -- but that can be patched over in much the same way. Unfortunately, doing so makes it less obvious that users on older versions of Node.js are doing the wrong thing -- and transparently fixing the issue in new versions of Node.js could lead to developers being completely unaware that their older version of Node.js may have an issue. That's not to say that zero-filling by default and limiting the maximize size to avoid DOS is not a good approach, it's just that there are still ecosystem and usability issues that go along with it.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Nov 10, 2016

@mafintosh For more context on what @jasnell said, see #4660 (comment) (from before Buffer.alloc existed). The concern is that this would actually create security issues, because people would stop using new Buffer(num).fill(0) in modules, and then anyone using an older version of Node without the zerofill behavior would be vulnerable.

This might be more doable now that Buffer.alloc exists. If we decide to go that route, I think we should at least keep Buffer() and new Buffer() soft-deprecated to avoid this issue and to encourage everyone to use Buffer.alloc instead.

@mafintosh

This comment has been minimized.

Copy link
Member

mafintosh commented Nov 10, 2016

Just to clarify: I'm all for the soft deprecating of the constructor. I was referring to doing the zero-fill instead of hard-deprecating it.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Nov 11, 2016

I do not think we should ever turn Buffer into an ES6 class; we can create a separate API for that and make Buffer a wrapper around that

I'm going to disagree with that. The additive approach of "just add a new API" leads to a sprawling and uncohesive design. Newcomers to node already complain there is so much to take in, let's not make it worse.

Accidentally accepting large numeric values can very quickly increase resource usage

v6.x and v7.x solve that by throwing an exception when calling Buffer(1234, 'encoding') (but not for Buffer('1234', 'encoding')), we should discuss back-porting that to v4.x.

I'm in favor, it improves security and I can only see it breaking code that is already prone to getting broken by attackers. Better it's us than some black hat, am I right?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 11, 2016

v6.x and v7.x solve that by throwing an exception when calling Buffer(1234, 'encoding') (but not for Buffer('1234', 'encoding'))

They don’t when there’s only a single argument, Buffer(1234) vs Buffer('1234').

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 11, 2016

I'm on record for supporting deprecation of Node.js APIs that don't make sense (https://www.youtube.com/watch?v=jJaIwea8r2A, https://gist.github.com/sam-github/4c5c019b92cf95fb6571), and I even support the deprecation of these Buffer APIs (slowly, properly communicated), but I think the pain of these kind of changes is not well appreciated.

For one, console messages have huge negative impacts downstream, because people who can do nothing about them see them, see #9483

But for another, trivial changes become less and less trivial as they work their way up module dependencies.

For example, say a version of glob is deprecated because there is a security problem. A new major comes out. The only change is in an obscure corner case I don't care about, and most don't. But, its a functional change, it must be a major. That's OK.

To update to the new major for packages that directly depend on glob is trivial, just bump the major in your package, the API didn't actually chnage, so no code changes. So far, so good. So packages that depend on glob do this, takes a couple minutes, republish, no problem.

But authors do this on only the head of their packages. Going back into history and doing a patch release of every major version that has ever used glob? That's a lot of work, so only the latest. And maybe they bump their package (EDIT: major) version because, hey, who knows, maybe some downstream dep depended on that particular glob corner case.

And the changes slowly work their way up, 3 levels up through other packages that have had major updates, used by tap.... and now the latest tap depends on the latest glob, which is A-OK.

But I have packages with unit tests I haven't touched in years... and now I need to bump to the latest tap to get ride of the messages about glob... and while glob's change was tiny and insignificant, tap has changed a lot, and all my tests are now failing, and I'm spending hours and hours fixing them, even though the original change in glob required no js code changes for the update!

This has been happening to me lately, with the graceful-fs and glob updates. I support those updates, and I'm doing the work, and I don't mind.

But its worth remembering that both those updates were at their root trivial, graceful-fs and glob had new versions that were drop-in replacements for the previous majors. So, upgrading should have been trivial, and it was for direct dependencies, but as the changes ripple up the dependency trees, the changes become less and less trivial, because they start to get bundled with changes that are not so trivial.

This is the kind of thing that will happen with the Buffer changes. Immediate code that uses Buffer will change trivially, but further up the tree, its going to hurt a lot more.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Nov 12, 2016

They don’t when there’s only a single argument, Buffer(1234) vs Buffer('1234').

Yes, and it's regrettable, but that can't be changed anytime soon. Back-porting the changes for the two argument case would at least mitigate @ChALkeR's hoek example.

@wmhilton

This comment has been minimized.

Copy link

wmhilton commented Nov 12, 2016

I've been thinking a lot about this, and actually written a bit but haven't shared my thoughts yet because they're incomplete. But I wanted to share one of my ideas: perhaps the fundamental problem had nothing to do with Buffers or APIs at all, but how we deprecate things.

There are two values that we all share but right now they are in conflict: Security and Stability. Core wants to motivate module authors to fix two highly dangerous flaws that might exist in their code: leaking secrets via uninitialized buffers, and DDOS attacks via buffers constructed with unboundchecked ints. Having created the flawed API, it seems reasonable they assumed responsibility for trying to fix the mistake. But we can't always solve the problems we create on our own, and this is a case where core had dug itself in a hole, and would need module authors to help lift them out. Because the Buffer API was a one-way (lossy) function: you could easily convert Buffer.from, Buffer.allocate, and Buffer.allocUnsafe to the unsafe "Buffer()" form, but given a Buffer() call it is impossible to automatically choose a safe behavior based the arguments, because knowing whether it was being used safely or not requires analyzing the source code in the calling function which the Buffer() function doesn't have access to.

When your only tool is a hammer, every problem looks like a nail. So they used the only tools they had: the docs website for node, and a console message in node. That's when we ran into a stability conflict. Because apparently the console output of Node is treated as part of it's API by test runners, which saw the deprecation message and freaked out. Which sort of succeeded at the original aim: get module authors attention so they can fix this flaw. However it flew in the face of another value: Stability. Module author's freaked out, thinking core was randomly changing the Buffer API, and complained loudly. And rightly so, because broken test cases, multiplied by every project that includes that somewhere as a dependency, creates chaos. (See leftpadgate)

The literal cause of the problem is not deprecating Buffer - it's how it was communicated, which turned out to break modules, much to core's surprise, I think. I certainly wouldn't have thought one little console log message would break builds. But apparently it does! So what can we do?

Core values stability just like module authors, I'm certain. Therefore it seems clear that deprecating things by printing to the console log is the wrong strategy, because that will always interfere with program output. I think that deprecating strategy... has to go. Should be removed from core's toolbox.

Now here's my actual novelty/contribution. If the goal of deprecation is to get module authors attention so they fix their code, node core is not the right venue for that. Npm is. I think, core should reach out to npm and see if they could help publicize deprecations. Place a warning banner on every module that uses the old unsafe API. This would have zero effect on how the code runs and therefore would not threaten the stability of the module ecosystem. Yes, developers might be pissed to be publicly called out for their module using insecure APIs. So take advantage of the fact that npm has the email address for all the authors and let them know in advance. Maybe just show a small warning that gets bigger over time. The point is, deprecation is fundamentally a social process done via communication, not a technical one that can be done by modifying the node engine.

Sorry if I rambled on a bit.

@seishun

This comment has been minimized.

Copy link
Member

seishun commented Nov 12, 2016

I certainly wouldn't have thought one little console log message would break builds. But apparently it does!

So far I've only heard of one module that was actually functionally broken by Buffer-without-new deprecation. All other cases of breakage I've encountered were in tests which look at stderr output.

Place a warning banner on every module that uses the old unsafe API.

Static analysis doesn't always work well in JavaScript.

@wmhilton

This comment has been minimized.

Copy link

wmhilton commented Nov 12, 2016

All other cases of breakage I've encountered were in tests which look at stderr output.

A broken test is a broken test, is a broken test. It is disruptive, especially if you rely on Continuous Deployment and a breaking test halts deploying to production. But that's a hypothetical on my part. I am a little curious though about the particulars. Can someone (@mafintosh @substack etc) speak to what horrors rained down on them as a consequence of test suites breaking?

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Nov 14, 2016

​Unexpected console output when using a node CLI program is a functional
breakage.

@feross

This comment has been minimized.

Copy link
Contributor

feross commented Nov 18, 2016

I (along with @mafintosh) were the original reporters of this issue. After reading through this discussion (and dealing with many issues opened by users about this issue), I think the best way forward is to zero-fill buffers created with Buffer() and new Buffer().

Proposal: Zero-fill buffers created with Buffer() and new Buffer() in v7 (as well as active LTS versions: v4 and v6)

This solves the most important issue that is at stake here: the possibility of accidental data leakage via an unintentional Buffer(num). The DOS issue of Buffer(really_large_num) remains, but denial-of-service is far, far better than data leakage.

Since Buffer() is already soft-deprecated in the docs, modules written in the future should not be using it. Existing code that uses Buffer() will only benefit from the zero-filling, which may fix existing security issues.

The risk that module authors will start assuming that Buffer(num) will zero-fill, leading to insecure modules on older Node versions is reduced if we ship zero-filling to all active LTS versions.

In conclusion, zero-filling fixes the most important issue – data leakage – without major breaking changes and without hard-deprecating the Buffer API.

This solution lets us support the legacy Buffer() API for the foreseeable future, ensuring existing code continues to work, without it being a security risk for new code that happens to use Buffer().

@seishun

This comment has been minimized.

Copy link
Member

seishun commented Nov 19, 2016

It seems there is consensus that security issues in old modules are a real problem, but some people think it could be better solved by zero-filling new Buffer(num) instead. While this alternative proposal has some merits, it also has its shortcomings, which in my opinion outweigh the merits. I'll try to summarize them below.

Solution A: one-time runtime warning (aka hard deprecation)

  • (+) Fixes DoS vulnerabilities by encouraging users to fix their code or update their dependencies.
  • (+) Actively used code will eventually migrate to the new API, making it less likely that people new to Node.js will ever need to know about or deal with the legacy API.
  • (+) Support for calling Buffer without new could eventually be removed, which allows Buffer to become a proper ES6 class that can be subclassed.
  • (-) Creates extra work for maintainers and annoyance for users. It may take a long time to sort it out in case of large dependency trees. Unmaintained modules might never update.

Solution B: zero-fill buffers created with Buffer(num) and new Buffer(num)

  • (?) If this is not backported, it might create new security vulnerabilities, see @ChALkeR's summary here.
  • (?) If this is backported, it will silently introduce performance degradation to some users, and they will have to search for usages of [new ]Buffer(num) manually since they won't have the equivalent of --trace-deprecation to help them. The backport also won't help users who for some reason will continue using old versions of Node.js. (Note: I'm all for forcing people using outdated versions of node to upgrade, but silently giving them security vulnerabilities is a different matter.)
  • (-) Raises the question why we didn't do this in the first place instead of introducing new APIs ("if new Buffer is OK in old code, then it should be OK in new code, so what's the point of Buffer.alloc*?"). Might create an impression of bad long-term planning.
@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Nov 19, 2016

There will not be a solution that please everyone. I'm definitely 👎 on zero-filling Buffer, and at the same time I am 👎 on hard deprecating anything. And I am 👎 on delivering insecure software. I am also aware that one of the above will happen. It's a bad situation.

Just to clarify, the major issue I see with the old API is Buffer(string) and new Buffer(string), but not new Buffer('hello world') and new Buffer(42). For old modules, the only problematic case is new Buffer(string), where string is a variable.

Emitting the warning just for Buffer(str), but not Buffer(str, 'utf8') and Buffer(int) might also be a viable option. This should reduce the surface area for maintainers to just the cases that are problematic.
It might be a silly idea.

Can we start adding rules in the linting/security software to catch this? Maybe there is already, but it might be an easy way to get started.

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Nov 19, 2016

Emitting the warning just for Buffer(str), but not Buffer(str, 'utf8') and Buffer(int) might also be a viable option. This should reduce the surface area for maintainers to just the cases that are problematic.

I also thought about that, but no, that won't work. Buffer(42, 'utf8') does the bad thing on 4.x LTS, so Buffer(str, 'utf8') is not universally safe. And adding a throw into v4.x LTS is hard to justify.

new Buffer(42) could probably be fine, though I am not yet sure, need to think about that a bit more.
Upd: at least one cons — if we keep it, we won't be able to zero-fill new Buffer(42) for the reasons noted below.

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Nov 19, 2016

@feross Re: zero-fill, the only viable time to zero-fill is in the same release that introduces a runtime deprecation warning for the API. E.g. when/if Buffer(42) is runtime-deprecated with a clear message, we will be sure that no people would add that in new code, thinking that it will zero-fill on all releases that they care about.

The problem with zero-fill is that if we introduce it in vN.0.0 but don't introduce a runtime-deprecation for Buffer(42) in that release, then at least some library authors caring about only vN.0.0 would rely on Buffer(42) being zero-filled, and when someone runs that code on a previous release, things will go even worse than they are now.

Also don't forget that zero-fill doesn't prevent DoS issues.

@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Nov 19, 2016

Re: zero-fill, the only viable time to zero-fill is in the same release that introduces a runtime deprecation warning for the API. E.g. when/if Buffer(42) is runtime-deprecated with a clear message, we will be sure that no people would add that in new code, thinking that it will zero-fill on all releases that they care about.

@ChALkeR Making things safe and deprecating an API are two separate things. By zero-filling security becomes guaranteed which is a different concern from migrating people onto a new API. I feel while the two might be related, they are by no means tied to each other.

@ChALkeR if I understand you correctly you try and make the point that if people don't migrate to a new API the npm ecosystem will be worse off than it currently is. Given that in the current situation nobody is using the new API this is not possible. Instead what we can do is make all buffer code instantly safe by zero-filling, and move the ecosystem forward in a giant leap.

Then there's the point of moving people onto a new API. I feel NodeJS has a good outreach through release notes, blog posts talks and other channels. More than enough to make people aware of a changed API. Besides that there's also an incentive for developers to pick up the new API in the form of improved performance. If we do this well all developers that care about performance will have the means and reasons to move to the new API, all without needing to make maintainers sad.

I hope this sounds reasonable; I'm in favor of zero-filling by default as it seems like the least problematic way forward. Fwiw I'd also be keen to see the perf implications of zero-filling, as I don't recall seeing any numbers so far and having them would be great to incentivize people to move to a new API.

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Nov 20, 2016

I hope this sounds reasonable; I'm in favor of zero-filling by default as it seems like the least problematic way forward. Fwiw I'd also be keen to see the perf implications of zero-filling, as I don't recall seeing any numbers so far and having them would be great to incentivize people to move to a new API.

console.time('new Buffer(1024)')
for (var i = 0; i < 10000000; i++) {
  new Buffer(1024)
}
console.timeEnd('new Buffer(1024)')

Here it is, roughly 2x:

$ node --zero-fill-buffers test.js
new Buffer(1024): 4329.844ms
$ node test.js
new Buffer(1024): 2562.717ms

(node v6.9.1)

IMHO making an old API slower is worse than emitting a warning, because you are "forcing" people to move, with the additional problem that the culprit is not easy to track. On the good side, tests will not cause warnings to be emitted.
It might be ok as a semver-major change together with a warning, but not in v4, v6 or v7.

I am still 👎 on zero-filling, as I am 👎 on the deprecation warnings. However, if we go for a deprecation warning, zero-filling any new Buffer() and Buffer() call is acceptable.

Maybe we can warn something like:

"new Buffer() and Buffer are now zero filled for security reasons, see LINK". In LINK, we show the new APIs, and how to move to them. If we can write something like standard --fix that does the job automatically, it would be of great help.

The above warning is annoying, but it might cause less worry than a full deprecation. It is not a generic "the api might go away", but it's positive: we have deprecated this api to make you safe, read this link to make this warning disappear.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Nov 20, 2016

Keep in mind that zero-filling is currently done in an unsophisticated way. Preallocating memory coupled with offloading zeroing to a separate thread should remove much of the overhead.

@feross

This comment has been minimized.

Copy link
Contributor

feross commented Nov 20, 2016

@bnoordhuis Keep in mind that zero-filling is currently done in an unsophisticated way.

Good to know! If there's only a negligible performance difference to the zero-filling, that seems the best option to me.

@feross

This comment has been minimized.

Copy link
Contributor

feross commented Nov 20, 2016

@seishun Most of the cons in your list aren't correct.

If this is not backported, it will create new security vulnerabilities, see @ChALkeR's summary

If we add zero-filling to supported release lines, then the impact will be minimal. Users on v4, v6, v7 (and 0.12 if this is resolved in time) will be covered.

As for users on 0.10 or other unsupported versions -- they're already playing with fire by running an unsupported version in product. Even so, they're unlikely to be affected by new code that uses Buffer() unsafely IMO. Buffer() is already deprecated in the docs. New code should not be using it. (Developers who are familiar with the old behavior and haven't read the docs recently will continue to assume that they are responsible for zeroing/overwriting the buffer, so no harm done.)

If this is backported, it will silently introduce non-trivial performance degradation to some users

This is not an issue. See @bnoordhuis's comment: #9531 (comment)

Raises the question why we didn't do this in the first place instead of introducing new APIs ("if new Buffer is OK in old code, then it should be OK in new code, so what's the point of Buffer.alloc*?"). Might create an impression of bad long-term planning.

The point of the new APIs (Buffer.alloc(), etc.) is to split apart two very different behaviors into explicit functions. Buffer.alloc() allocates num bytes of memory. Buffer.from() converts an object into a Buffer.

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Nov 20, 2016

@feross

Users on v4, v6, v7 (and 0.12 if this is resolved in time) will be covered.

No, they won't. You are assuming an immediate update, I expect the number of users that will gain problems from this (i.e. that don't update immediately to a latest version in the branch, or don't update fast enough before they install a package that relies on zero-filling) as non-zero and significant.
Moreover, I estimate this to be more disturbing on the ecosystem than forcing maintainers of popular packages to do trivial updates.

Also, every time someone proposes zero-filling, they are ignoring the DoS issue.

@seishun

This comment has been minimized.

Copy link
Member

seishun commented Nov 20, 2016

Developers who are familiar with the old behavior and haven't read the docs recently will continue to assume that they are responsible for zeroing/overwriting the buffer, so no harm done.

Why do you think these developers won't make the same mistakes that triggered the creation of the new Buffer API in the first place?

This is not an issue. See @bnoordhuis's comment: #9531 (comment)

I wouldn't be so hasty with conclusions. Remember that people run Node.js on all kinds of hardware.

The point of the new APIs (Buffer.alloc(), etc.) is to split apart two very different behaviors into explicit functions.

No, splitting them apart was not the end goal. The end goal was to fix security and DoS vulnerabilities, and we agreed that it should be solved by introducing new API rather than changing the old one. Zero-filling now means backing down on that decision.

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Mar 12, 2017

@jasnell, thinking about it again, #9531 (comment) looks good to me. If we have a concrete short path to deprecation, random-fill is acceptable. I'm still not sure should it be backported or not, though — your plan doesn't include backporting. Perhaps that's the best — that way, the impact of «users start relying on randomfill» concern is reduced to the possible minimum, and 8.0 will be safe from uninitialized memory leaks.

It's still not neglectible, though — even now, module authors often just don't see case 3 from #9531 (comment) as an issue in the module.

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Mar 15, 2017

Sorry about being absent from this discussion for the last month or so. Can someone clarify if all the "runtime deprecation" options I was shown on the "Node.js Buffer options" spreadsheet mean completely deprecation of the Buffer constructor?

I've voiced this several times in the past, but my opinion is that if any changes are to occur (outside of only zero-filling) then Buffer parameters should become the same as or a super-set of Uint8Array. The requiring of new is also important. Is this reflected in any of those options?

To reiterate, new Buffer(number) and similar should never be runtime deprecated.

If the goal is to make Buffer more extensible, well, it's already plenty extensible, and I don't see how "Buffer can be subclassed" makes Node.js a better platform. In fact, it's probably a bad idea. The API should gently communicate that Buffer probably shouldn't be subclassed, and leaving it as a factory function accomplishes that.

That's a strong opinion @isaacs, but I don't see any reasoning or support for it. In the past I've extended Buffer with additional functionality that made the API much easier to work with. The key is being able to call functions on the instance, while also being able to access its data via array index. This is difficult to replicate, and it would make things much easier if I could simply use class Foo extends Buffer.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 15, 2017

Can someone clarify if all the "runtime deprecation" options I was shown on the "Node.js Buffer options" spreadsheet mean completely deprecation of the Buffer constructor?

Yeah, it does.

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Mar 15, 2017

@trevnorris, note that the table has a «to the technically possible extent» sentence, which means that it shouldn't break code that doesn't use Buffer(arg) explicitly. #7152 and #11808 have a work-around for that, and everything works as far as I am informed. That was done by @seishun, I believe. Could you provide a testcase that would be broken by any of those PRs? It's better to move the dicussion to the PRs, though.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 18, 2017

Upgrading this one from ctc-review to ctc-agenda. We need to make a decision about what is and isn't going to happen in version 8.0.0.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 26, 2017

Summing up where things are now, at least as I see them, and reading a lot into the spreadsheet @ChALkeR set up and that all CTC members were invited to fill out:

  • While not unanimous, there seems to be a consensus that we should do something--that ignoring the issue is not a wise option.

  • Deprecating in version 8.0.0 has considerable opposition and scant support.

  • Zero-filling has more support than opposition at this time. That said, there are almost as many people who are neutral about it as there are people that are for it. So a lot will depend on how those folks end up voting.

  • Scheduling a deprecation has significant support and slightly less opposition. Again, a lot of neutral folks on that one, so how that goes will depend how the undecideds end up voting.

  • Opt-in deprecate is expected to easily land in version 8.0.0. It has two people on the record as being opposed. One of them is opposed to anything other than updating the docs. If I recall correctly, the other had trouble remembering why they indicated opposition and it may have been an error.

  • For some folks, support or opposition to zero-fill depends on whether there is a commitment to run-time deprecating (for example, announcing that run-time deprecation will happen in version N.0.0) and whether it will be backported to LTS lines. Therefore, it may make sense to try to come to a decision on whether or not to schedule a deprecation before trying to decide whether or not to zero-fill.

  • Random-fill seems to have less support than zero-fill but is still a viable contender. Most of the things said about zero-fill

@addaleax addaleax removed this from the 8.0.0 milestone Apr 1, 2017

@Trott Trott removed the ctc-agenda label Apr 3, 2017

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Apr 3, 2017

I think this has been resolved, at least for Node.js 8.0.0. We will want to revisit this in 6 months (if not sooner!) before the Node.js 9.0.0 release.

For now, the CTC has decided that:

  • Node.js 8.0.0 will contain a flag allowing people to opt-in for a runtime deprecation message for Buffer constructor usage. (Will people actually use it? We'll find out.)
  • Node.js 8.0.0 will zero-fill buffers created with the Buffer constructor. This behavior, at least for the time being, will not be backported to earlier versions.

There were no decisions that were going to please everyone. This is where things sit for now. As mentioned above, we'll surely be re-visiting this in the not-too-distant future to assess how things are working or not working.

I should also mention that there is an effort to get a rule into ESLint that will flag Buffer constructor usage. I would characterize the state of that proposal as likely to be adopted by ESLint, but not a sure thing at this time.

I'm going to close this issue, but feel free to re-open or comment if you think that's not the right thing to do at this time. Thanks!

@Trott Trott closed this Apr 3, 2017

@feross

This comment has been minimized.

Copy link
Contributor

feross commented Apr 5, 2017

FYI, I just released standard 10.0.0 which treats usage of deprecated Node.js APIs as a lint error. So we now have thousands of users (once they update) who will see warnings about Buffer() being deprecated in their tests and in their CI pipelines.

From the 10.0.0 changelog entry:

  • Disallow using deprecated Node.js APIs
    • Ensures that code always runs without warnings on the latest versions of Node.js
    • Ensures that safe Buffer methods (Buffer.from(), Buffer.alloc()) are used instead of Buffer()

It's hard to know exactly how many people use standard, but our shareable eslint config is downloaded 670K times per month, so I hope this change will have some noticeable effect in the usage numbers. We'll see.

I think it would be great if we could lean on community tooling, like standard and others, to help make these kinds of deprecations less painful in the future. @ChALkeR, it would be great if you could keep an eye on usage of Buffer() to see how much it changes over the next 1-3 months.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Apr 5, 2017

Also on the tooling front: ESLint will be shipping with a no-buffer-constructor (that may not be the name of the rule, I'm just using that as a shorthand right now) rule in the foreseeable future. See eslint/eslint#5614 (comment) and thank @not-an-aardvark and @jasnell and everyone else who got us to this point. (I don't know if the rule will have enormous impact or modest impact, but we don't know if we don't try!)

@seishun

This comment has been minimized.

Copy link
Member

seishun commented Jul 22, 2017

With the Node.js 9.0.0 release looming closer, I think it's time to revisit this. @ChALkeR could you evaluate how much the usage of Buffer() has changed in the last 3 months?

@ChALkeR

This comment has been minimized.

Copy link
Member

ChALkeR commented Jul 22, 2017

@seishun Thanks for the reminder! I hope to do that in a few days. =)

@seishun

This comment has been minimized.

Copy link
Member

seishun commented Aug 15, 2017

@ChALkeR pinging once again...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment