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.

Sign up for GitHub

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

Jump to bottom

buffer: backport --zero-fill-buffers command line option #5744

Closed
wants to merge 1 commit into from
Closed

buffer: backport --zero-fill-buffers command line option #5744

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 16, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

buffer

Description of change

This backports the --zero-fill-buffers command line flag introduced in master. When used, all Buffer and SlowBuffer instances will zero fill by default.

This does not backport any of the other Buffer API or behavior changes.

Note: My intent is to open similar backports for --zero-fill-buffer in v4, v0.12 and v0.10

/cc @trevnorris

@jasnell jasnell added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. v5.x labels Mar 16, 2016
@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2016

@nodejs/ctc

@Fishrock123
Copy link
Contributor

Uh, why not land the actual patches with the new APIs?

@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2016

Because it's technically a semver-major even tho it's a soft deprecation and there's been no decision about pulling all of the APIs back. I'm certainly open to the discussion but we'd need to decide how far back to backport

@ChALkeR
Copy link
Member

ChALkeR commented Mar 17, 2016

We can land those without the soft deprecation, but it should be discussed in a separate issue, probably.

+1 to these PRs that implement the --zero-fill-buffers flag (for all versions). I did not yet review the code, will do that later today.

@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2016

I'm +1 with backporting the new APIs to v5 without the soft deprecation.
The @nodejs/lts WG and @nodejs/ctc would need to discuss and decide if it makes sense to backport the new APIs to v4/v0.12/v0.10.

At the very least, however, let's get the --zero-fill-buffers flag landed in each.

@Fishrock123
Copy link
Contributor

Everything except the deprecation should (i.e. needs to) land in v5.x -- the deprecation should have been in a separate commit.

@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2016

That would be easy enough to do. I'll get the v5 PR updated this morning.
Any opinions about backporting the APIs to v4?
On Mar 17, 2016 7:10 AM, "Jeremiah Senkpiel" notifications@github.com
wrote:

Everything except the deprecation should (i.e. needs to) land in v5.x --
the deprecation should have been in a separate commit.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#5744 (comment)

@ChALkeR
Copy link
Member

ChALkeR commented Mar 17, 2016

@jasnell it would be good to include the new API in the next v4 minor, imo.

@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2016

Adding lts-agenda so the @nodejs/lts WG can discuss backporting the APIs to v4

@jasnell jasnell mentioned this pull request Mar 17, 2016
Closed
4 tasks
@ChALkeR
Copy link
Member

ChALkeR commented Mar 19, 2016

LGTM

1 similar comment
@trevnorris
Copy link
Contributor

LGTM

@jasnell jasnell mentioned this pull request Mar 21, 2016
Closed
@jasnell
Copy link
Member Author

jasnell commented Mar 21, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/2001/

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

Discussion by the CTC today was to get this landed in v5 but hold off backporting further.

@jasnell
buffer: backport --zero-fill-buffers command line option
c64d0b0 
This backports the --zero-fill-buffers command line flag introduced
in master. When used, all Buffer and SlowBuffer instances will zero
fill by default.

This does *not* backport any of the other Buffer API or behavior
changes.
@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

@Fishrock123 ... updated this to fix up doc/node.1 and doc/api/cli.markdown. PTAL

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2047/

@jasnell
Copy link
Member Author

jasnell commented Mar 23, 2016

CI is green except for one unrelated failure.

jasnell added a commit that referenced this pull request Mar 24, 2016
@jasnell
buffer: backport --zero-fill-buffers command line option
3c02727 
This backports the --zero-fill-buffers command line flag introduced
in master. When used, all Buffer and SlowBuffer instances will zero
fill by default.

This does *not* backport any of the other Buffer API or behavior
changes.

PR-URL: #5744
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Mar 24, 2016

Landed in v5.x in 3c02727

@jasnell jasnell closed this Mar 24, 2016
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
Closed
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. (Forrest L Norvell)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)

PR-URL: #5970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jasnell @Fishrock123 @ChALkeR @trevnorris