Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

Buffer.from/Buffer.alloc/Buffer.allocUnsafe/Buffer() soft-deprecate #7

Closed
wants to merge 1 commit into from
Closed

Buffer.from/Buffer.alloc/Buffer.allocUnsafe/Buffer() soft-deprecate #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions XXX-buffer-alloc-from.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Buffer.alloc / Buffer.allocUnsafe / Buffer.from

Status | Draft
------ | ----
Author | James M Snell
Date | 25 Jan 2016

## Purpose

* Soft-Deprecate the existing `Buffer()` constructors and introduce new
`Buffer.from()`, `Buffer.alloc()` and `Buffer.allocUnsafe()` factory methods
to address Buffer API usability and security concerns.
* Add `--zero-fill-buffers` command line option to node to force all Buffers
to zero-fill when created

## Details

The details of the proposal are captured by [Pull Request 4682][] in the
nodejs/node repository (https://github.com/jasnell/node/tree/buffer-safe).

### Buffer.alloc(size[, fill])

Allocates a new initialized buffer of `size` bytes. If `fill` is specified
and is a string, the Buffer is allocated off the shared pool by calling:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and is a string

I would expect it to work as Buffer(size).fill(fill) does, in all cases (everything else is implementation details), except for the cases where .fill() doesn't work (e.g. .fill('') doesn't fill).

For example, I would expect Buffer.alloc(2, 10) to give the same result as Buffer(2).fill(10) — <Buffer 0a 0a>.

Also, is Buffer.alloc(1e8) really allocated from the pool?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is Buffer.alloc(1e8) really allocated from the pool?

Allocations are of size Buffer.poolSize >> 1 or smaller. So it could, depending on what the user sets poolSize to. But by default no.

@jasnell Since we're here, may want to add Buffer.alloc(size[, fill[, encoding]]) so the encoding of the string can be specified. I should have done this on initial implementation but didn't think about it then.

Also, if alloc() is used then I suggest we also don't use a pool. Since other Buffer's contents can be accessed via buffer.buffer it might be considered a security violation, and the whole point of this thing is to not do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris ... +1 SGTM
On Jan 25, 2016 4:27 PM, "Trevor Norris" notifications@github.com wrote:

In XXX-buffer-alloc-from.md
#7 (comment):

+* Soft-Deprecate the existing Buffer() constructors and introduce new

  • Buffer.from(), Buffer.alloc() and Buffer.allocUnsafe() factory methods
  • to address Buffer API usability and security concerns.
    +* Add --zero-fill-buffers command line option to node to force all Buffers
  • to zero-fill when created

+## Details
+
+The details of the proposal are captured by [Pull Request 4682][] in the
+nodejs/node repository (https://github.com/jasnell/node/tree/buffer-safe).
+
+### Buffer.alloc(size[, fill])
+
+Allocates a new initialized buffer of size bytes. If fill is specified
+and is a string, the Buffer is allocated off the shared pool by calling:

Also, is Buffer.alloc(1e8) really allocated from the pool?

Allocations are of size Buffer.poolSize >> 1 or smaller. So it could,
depending on what the user sets poolSize to. But by default no.

@jasnell https://github.com/jasnell Since we're here, may want to add Buffer.alloc(size[,
fill[, encoding]]) so the encoding of the string can be specified. I
should have done this on initial implementation but didn't think about it
then.

Also, if alloc() is used then I suggest we also don't use a pool. Since
other Buffer's contents can be accessed via buffer.buffer it might be
considered a security violation, and the whole point of this thing is to
not do that.

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node-eps/pull/7/files#r50779681.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris

But by default no.

I made a mistype, was talking about Buffer.alloc(1e8, 'a'). If that's not allocated off the shared pool, then the text here should be changed:

If fill is specified and is a string, the Buffer is allocated off the shared pool by calling

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh... yep, I keep forgetting the little nuances there of what is pooled vs. what isn't. I'll get the docs updated properly in the next round :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trevnorris ... buf.fill() does not currently accept an encoding argument does it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell it doesn't. i'm working on that patch right now to fix the API hole.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just want to echo @ChALkeR's point that buf.fill(val) currently accepts number values and this API should do that too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the proposal PR has been updated to account for that. For
completeness, @trevnorris and I are also working on allowing both fill()
and alloc(size, fill) to accept a Buffer and to accept an encoding
when a string is passed in, e.g:

// these are silly examples, but you should get the idea:
Buffer.alloc(10, 'foo', 'ascii');
Buffer.alloc(10, Buffer.from('foo'));
Buffer.alloc(10).fill('foo', 'ascii');
Buffer.alloc(10).fill(Buffer.from('foo'));


```
Buffer.allocUnsafe(size).fill(fill);
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. Instead a simple call to new Uint8Array(), without setting the kNoZeroFill flag, and simply setting the prototype of the uint8array to Buffer.prototype, using Object.setPrototypeOf().

EDIT: nm. read this out of context. the above simply states the allocation mechanics. which is also stated below.


Otherwise, the buffer is initialized by allocating new zero-filled memory

### Buffer.allocUnsafe(size)

Allocates a new uninitialized buffer of `size` bytes off the shared pool. This
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise — is this always the pool?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note response above about size of pool allocations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Also may want to make a note about usage of Buffer.poolSize here. For clarification of how the mechanics work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the text should be updated here, too =).

is equivalent to calling `new Buffer(size)`

### Buffer.from(value[, encoding])

Replaces the existing `new Buffer(value[, encoding])` constructors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we throw if typeof value === 'number'? If so that should be stated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could either throw or cast numbers to strings. Not sure which is better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps throwing, because one could argue that Buffer.from(10) should be <Buffer 10> instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer throwing given that the issue we're trying to fix deals with ambiguous argument types

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should throw here. Buffer.from(10) could mean a few different things. Did the user mean Buffer.alloc(10)? Or perhaps Buffer.from([10])? Or even Buffer.from('10')? Hard to say.


### `--zero-fill-buffers` command line option

Forces all Buffers to be zero-filled upon allocation.

## Related Discussion

* https://github.com/nodejs/node/issues/4660
* https://github.com/ChALkeR/notes/blob/master/Lets-fix-Buffer-API.md

[Pull Request 4682]: https://github.com/nodejs/node/pull/4682