buffer: runtime-deprecate Buffer(num) by default

[seishun]

This is a "light" version of #15346 that only prints the warning by default when [new ]Buffer(num) is used, as suggested by @BridgeAR in #15346 (comment).

Most reasons for runtime deprecation by default as listed in #15346 still apply here.

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

[addaleax]

If we ever decide to runtime-deprecate something, it should be the Buffer(string) overload, because that is the one that led to the issues with the Buffer constructor in the first place.

[BridgeAR]

@addaleax you mean to deprecate the usage of the buffer constructor with a single argument passed in? I also thought about that as only warning in case a number is passed is actually not what helps out anyone who regularly passes in a string and accepts user input for that.
The issue is that this is probably a common pattern and I thought it would probably be best to warn the users in case a number was passed to the constructor. In that case the user could at least potentially realize a misuse when occurring (even if that is not very likely).

I am open for deprecating Buffer(string) in general, I am just not sure if that will find a majority of votes.

[addaleax]

@BridgeAR When I write Buffer(string) I mean the Buffer constructor with a single argument that has typeof arg === string :)

The issue is that this is probably a common pattern and I thought it would probably be best to warn the users in case a number was passed to the constructor.

Both variants are common patterns, which is why I’m still -1 on runtime-deprecations in general.

In that case the user could at least potentially realize a misuse when occurring (even if that is not very likely).

I agree, it’s unlikely that this would help more than a handful people, if anyone.

[Trott]

It won't persuade everyone, but I would be interested to know how much (if any) breakage a CITGM run with this causes compared to "full" deprecation.

[seishun]

@Trott well, CITGM fails on master, so I doubt running it with this would be informative.

[Trott]

This might be is almost certainly a terrible suggestion that will get support from neither side on this issue, but is there merit in considering "issue a deprecation warning when num is greater than <some large-ish value>"?

[jasnell]

@Trott ... that would not address the issues with the API. That is, the problem is not with Buffer(largeNumber) but Buffer(anyNumber) when it's not at all clear that anyNumber is actually a number or not ;-)

[Trott]

@Trott ... that would not address the issues with the API.

@jasnell It addresses one of the issues with the API (the DoS issue).

That is, the problem is not with Buffer(largeNumber) but Buffer(anyNumber) when it's not at all clear that anyNumber is actually a number or not ;-)

Correct that it does not address the type-confusion issue. Looking at it through a the lens of people like @seishun who think a runtime deprecation is warranted, "It doesn't address everything" is not terribly persuasive if the only alternative is to address nothing at all.

[Trott]

@jasnell Although thinking about it more...the DoS issue is only an issue with type confusion in the first place. And switching to Buffer.alloc() won't fix the DoS issue (although switching to Buffer.from() would).

In any event, I do find it plausible that the cost of such a limited runtime deprecation may exceed its benefits.

Hey, I did say it was probably a terrible idea. 🙃

[Trott]

@seishun Since 9.0.0 is out, should this be closed? Rebased and targeted for 10.0.0? Remain open pending something else ?

[seishun]

If there is consensus that Buffer(num) won't be runtime-deprecated alone (aka either all forms of Buffer() will be deprecated together, or none) then I agree to close.

[BridgeAR]

Let us put it on the TSC agenda. There was not much discussion here, so I guess it will be best to force it a bit. That way we have more clarity.

[Trott]

Let us put it on the TSC agenda. There was not much discussion here, so I guess it will be best to force it a bit. That way we have more clarity.

Ref: #15346 (comment) and subsequent conversation after that comment

[Trott]

Since this is back on the agenda: /ping @nodejs/tsc

@BridgeAR @seishun Is this the narrow question needing immediate attention?:

• If and when Buffer constructor is deprecated, might Buffer(num) be deprecated before other forms of Buffer constructor? Or will they be deprecated all at once?

[seishun]

@Trott that looks good to me.

[addaleax]

If and when Buffer constructor is deprecated, might Buffer(num) be deprecated before other forms of Buffer constructor?

My answer to that is no, because:

• The overload of the Buffer constructor that can not be relied on is Buffer(string): It is when people intended to use the latter one that an information leak was possible in previous Node versions/a possible DoS vector was present.
• The Buffer(num) variant is consistent with the TypedArray spec, and since Buffer is a typed array, we should make it available.

[ChALkeR]

The Buffer(num) variant is consistent with the TypedArray spec, and since Buffer is a typed array, we should make it available.

That's not entirely correct — on older Node.js versions, Buffer(num) allocates non-zero-filled arrays, so deprecating Buffer(num) would benefit the users of those old Node.js versions once they get new modules that migrate from Buffer(num) as a result of that deprecation.

That said, I am not sure yet if it makes sense to get this in before deprecating Buffer(string). In fact, my current position is that both should be deprecated with 10.0 release (once 4.x goes unsupported).
Either way, we won't be able to runtime-deprecate this by default (i.e. without a flag) on 9.x.

[MylesBorins]

I would like to see this run against citgm

[Trott]

Consensus of TSC is that if we're going to runtime deprecate, we won't do a phased approach like this. Let's focus further conversation on the full runtime deprecation proposal in #15346. Thanks for opening this, @seishun.