buffer: zero-fill buffer allocated with invalid content #17428

addaleax · 2017-12-02T15:34:06Z

@cjihrig Sorry, I was just finishing this up when I saw your PR – I think your suggestion makes sense, but I think we should make Buffer.alloc() never return uninitialized data on any release line.

Zero-fill when Buffer.alloc() receives invalid fill data.

A solution like #17427 which switches
to throwing makes sense, but is likely a breaking change.

This suggestion leaves the behaviour of buffer.fill() untouched,
since any change to it would be a breaking change, and lets
Buffer.alloc() check whether any filling took place or not.

Refs: #17427
Refs: #17423

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

Zero-fill when `Buffer.alloc()` receives invalid fill data. A solution like nodejs#17427 which switches to throwing makes sense, but is likely a breaking change. This suggestion leaves the behaviour of `buffer.fill()` untouched, since any change to it would be a breaking change, and lets `Buffer.alloc()` check whether any filling took place or not. Refs: nodejs#17427 Refs: nodejs#17423

addaleax · 2017-12-02T15:36:12Z

@nodejs/buffer @nodejs/security The cat is kind of out of the bag with the public issue & the other PR being opened, but I do think this potentially has security implications and we should apply it (or some other solution that we agree on) to all release lines in the near future.

Edit: Okay, funnily enough, Node 4 + 6 already do throw an error in these situations, so this only remains for 8 and 9.

CI: https://ci.nodejs.org/job/node-test-commit/14528/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1123/

cjihrig

I'm fine with this for older/current release lines. I still think moving forward that throwing in fill() is the correct thing to do.

apapirovski

LGTM

apapirovski · 2017-12-02T15:43:14Z

doc/api/buffer.md

@@ -1254,6 +1259,19 @@ Example: Fill a `Buffer` with a two-byte character
 console.log(Buffer.allocUnsafe(3).fill('\u0222'));
 ```

+If `value` is contains invalid characters, it is truncated; if no valid


Nit: extra 'is' before contains

apapirovski · 2017-12-02T15:47:35Z

doc/api/buffer.md

+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/17428
+    description: Specifying an invalid string for `fill` now yields a


Just a thought but given yield is a JS keyword with a specific meaning and not necessarily a super familiar word for non-native speakers, is there another word we could use? Maybe results in, creates or returns?

You can say the same about returns ;) I’m happy with anything, results in sounds fine to me.

lpinca · 2017-12-03T13:49:23Z

doc/api/buffer.md

+// Prints: <Buffer aa aa aa aa aa>
+console.log(buf.fill('aazz', 'hex'));
+// Prints: <Buffer aa aa aa aa aa>
+console.log(buf.fill('zz', 'hex'));


~~Nit: missing // Prints: ?~~

Edit: nvm.

It seems the comments cover the next line, not the previous.

Yes, I misread, sorry.

targos · 2017-12-04T09:26:20Z

lib/buffer.js

@@ -266,7 +266,9 @@ Buffer.alloc = function alloc(size, fill, encoding) {
    // be interpreted as a start offset.
    if (typeof encoding !== 'string')
      encoding = undefined;
-    return createUnsafeBuffer(size).fill(fill, encoding);
+    const ret = createUnsafeBuffer(size);
+    if (fill_(ret, fill, encoding))


nit: if (fill_(ret, fill, encoding) > 0) { ? To avoid an unnecessary ToBoolean.

@targos yup, will fix while landing

addaleax · 2017-12-05T11:46:26Z

Landed in b31626e, 69a68c0

PR-URL: #17428 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Zero-fill when `Buffer.alloc()` receives invalid fill data. A solution like #17427 which switches to throwing makes sense, but is likely a breaking change. This suggestion leaves the behaviour of `buffer.fill()` untouched, since any change to it would be a breaking change, and lets `Buffer.alloc()` check whether any filling took place or not. PR-URL: #17428 Refs: #17427 Refs: #17423 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#17428 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Notable changes: * **buffer**: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * **deps**: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17531

This is a security release. All Node.js users should consult the security release summary at https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2017-15896 * CVE-2017-15897 * CVE-2017-3738 (from the openssl project) Notable Changes: * buffer: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * deps: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17532

Notable changes: * **buffer**: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * **deps**: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17531

This is a security release. All Node.js users should consult the security release summary at https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2017-15896 * CVE-2017-15897 * CVE-2017-3738 (from the openssl project) Notable Changes: * buffer: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * deps: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17532

Notable changes: * **buffer**: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * **deps**: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17531

This is a security release. All Node.js users should consult the security release summary at https://nodejs.org/en/blog/vulnerability/december-2017-security-releases/ for details on patched vulnerabilities. Fixes for the following CVEs are included in this release: * CVE-2017-15896 * CVE-2017-15897 * CVE-2017-3738 (from the openssl project) Notable Changes: * buffer: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * deps: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17532

Notable changes: * **buffer**: * buffer allocated with an invalid content will now be zero filled (Anna Henningsen) #17428 * **deps**: * openssl updated to 1.0.2n (Shigeki Ohtsu) #17526 PR-URL: #17531

doc: warn against filling buffer with invalid data

a4065eb

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 2, 2017

addaleax force-pushed the buffer-fill branch from c582673 to 19e2e14 Compare December 2, 2017 15:34

addaleax added lts-watch-v4.x security Issues and PRs related to security. labels Dec 2, 2017

jasnell approved these changes Dec 2, 2017

View reviewed changes

cjihrig approved these changes Dec 2, 2017

View reviewed changes

apapirovski approved these changes Dec 2, 2017

View reviewed changes

addaleax added 2 commits December 2, 2017 16:52

[squash] apapirovski nit

fcb1524

[squash into first commit] apapirovski nit

b9b4d29

addaleax added dont-land-on-v4.x and removed lts-watch-v4.x labels Dec 2, 2017

ofrobots approved these changes Dec 2, 2017

View reviewed changes

tniessen approved these changes Dec 2, 2017

View reviewed changes

lpinca approved these changes Dec 3, 2017

View reviewed changes

targos approved these changes Dec 4, 2017

View reviewed changes

addaleax mentioned this pull request Dec 5, 2017

Add remark dependencies #17320

Closed

addaleax closed this Dec 5, 2017

addaleax deleted the buffer-fill branch December 5, 2017 11:46

evanlucas mentioned this pull request Dec 7, 2017

v9.2.1 release proposal #17531

Merged

MylesBorins mentioned this pull request Dec 7, 2017

v8.9.3 proposal #17532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: zero-fill buffer allocated with invalid content #17428

buffer: zero-fill buffer allocated with invalid content #17428

addaleax commented Dec 2, 2017

addaleax commented Dec 2, 2017 •

edited

Loading

cjihrig left a comment

apapirovski left a comment

apapirovski Dec 2, 2017

apapirovski Dec 2, 2017 •

edited

Loading

addaleax Dec 2, 2017

lpinca Dec 3, 2017 •

edited

Loading

vsemozhetbyt Dec 3, 2017

lpinca Dec 3, 2017

targos Dec 4, 2017

addaleax Dec 5, 2017

addaleax commented Dec 5, 2017

buffer: zero-fill buffer allocated with invalid content #17428

buffer: zero-fill buffer allocated with invalid content #17428

Conversation

addaleax commented Dec 2, 2017

Checklist

Affected core subsystem(s)

addaleax commented Dec 2, 2017 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Dec 2, 2017

Choose a reason for hiding this comment

apapirovski Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax Dec 2, 2017

Choose a reason for hiding this comment

lpinca Dec 3, 2017 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Dec 3, 2017

Choose a reason for hiding this comment

lpinca Dec 3, 2017

Choose a reason for hiding this comment

targos Dec 4, 2017

Choose a reason for hiding this comment

addaleax Dec 5, 2017

Choose a reason for hiding this comment

addaleax commented Dec 5, 2017

addaleax commented Dec 2, 2017 •

edited

Loading

apapirovski Dec 2, 2017 •

edited

Loading

lpinca Dec 3, 2017 •

edited

Loading