buffer: remove noAssert argument #12119

bnoordhuis · 2017-03-29T13:04:59Z

noAssert=true is intrinsically unsafe. Always-on checking does
cause a performance regression but some of that can be regained by
open-coding the checks (which this pull request doesn't do but can
be done at a later time if so desired.)

What's more, I don't think performance is ultra-critical for these
methods. If you want to squeeze out every last ounce of oomph, you
don't call out to generic kitchen-sink methods, you open-code loads
and stores directly in your parser or generator. I can see no good
reason for leaving it in and that is why this commit takes it out.

Refs: #8724 (#8724 (comment) is relevant vis-a-vis performance.)

Trott · 2017-03-29T13:50:47Z

test/parallel/test-buffer-arraybuffer.js

@@ -43,12 +43,16 @@ assert.throws(function() {
  Buffer.from(new AB());
 }, TypeError);

-// write{Double,Float}{LE,BE} with noAssert should not crash, cf. #3766
+// https://github.com/nodejs/node/issues/3766


Since this GitHub issue is specifically about behavior when noAssert is true, I think the URL/comment can be removed.

Trott · 2017-03-29T13:51:26Z

test/parallel/test-buffer-arraybuffer.js

-b.writeDoubleLE(11.11, 0, true);
-b.writeDoubleBE(11.11, 0, true);
+assert.throws(() => b.writeFloatLE(11.11, 0),
+              /RangeError: out of range index/);


I think these four tests can be removed if you want, as they were a regression test for a crash when noAssert was true. No noAssert means no regression possible. :-D

Trott · 2017-03-29T13:52:36Z

test/parallel/test-buffer-arraybuffer.js

+assert.throws(() => b.writeFloatLE(11.11, 0),
+              /RangeError: out of range index/);
+assert.throws(() => b.writeFloatBE(11.11, 0),
+              /RangeError: out of range index/);


If you decide to keep these four tests, though, please add a ^ and $ to the start and end respectively of the RegExp.

mscdex · 2017-03-29T16:08:10Z

-1 If noAssert gives ~3.5x performance, why remove the option when it's not causing any problems right now and it's being used responsibly? Not having to copy and paste these implementations into each project/module is a win IMHO, especially if optimizations to node's implementation are made over time, third party copies would then become stale and slower on newer node versions.

bnoordhuis · 2017-03-29T16:25:26Z

The security mailing list gets emails from people who think they found a security flaw or a denial of service when in fact they are passing $some_truthy_value for noAssert. It happens frequently enough that it stands out. I'd say that fact alone suggests it's a poorly designed API that causes real-world issues.

I also don't really buy the performance argument. When you use the built-in functions you do it for convenience. You don't really care if it takes 50 ns or 100 ns because if you did, you would use hand-coded versions.

(That said, see #8724 (comment) - I'm fairly sure the use of new Function() does something strange to the benchmarks that makes the performance gap look worse than it really is.)

Compromise: remove noAssert and add unsafeWriteUInt8() etc. methods? That leaves little to the imagination.

sam-github · 2017-03-29T18:40:33Z

I think the noAssert flag is very suspicious, and given the simple conditional check, its hard to see why its 3.5x slower.

As a positional arg, noAssert is easy to specify unintentionally, I like the idea of moving the unchecked array access into another API if they can't be deleted outright.

Aside: I really dislike the use of "safe" in APIs, it doesn't give any clue as to meaning. I'd prefer something more descriptive, like "unchecked". Even better would be to more all the APIs into a different module buffer.unchecked.writeUint8(). They wouldn't be as easy to call as methods, but why make writing unchecked code easy?

sam-github · 2017-03-29T18:41:42Z

It would be interesting to find an actual module that uses noAssert, maybe one of the DB procotols, and benchmark its real-world code with/without the check to see actual performance impact.

jasnell · 2017-03-29T20:16:12Z

Given that this change would cause code that currently works successfully when noAssert is truthy to potentially begin failing, I don't think we can do this without a proper deprecation cycle. I am perfectly fine with eliminating the additional option at some point, but I do not believe we should do so without deprecatiing.

bnoordhuis · 2017-03-29T20:36:08Z

That takes too long, I don't have the patience to sit that out.

Middle ground: make the OOB checks in src/node_buffer.cc unconditional. That should be uncontroversial because "code that currently works successfully" doesn't apply to code that violates memory safety.

cjihrig

Other arguments aside, the code changes LGTM.

jasnell · 2017-03-30T15:07:43Z

I could live with that approach.

TimothyGu · 2017-03-30T16:08:07Z

src/node_buffer.cc

-    else
-      memcpy_num = ts_obj_length - offset;
+    THROW_AND_RETURN_IF_OOB(false);
+    memcpy_num = ts_obj_length - offset;
  }


Uhh, the memcpy_num assignment is dead code since THROW_AND_RETURN_IF_OOB will always execute because of false.

Trott · 2017-08-05T21:48:35Z

@bnoordhuis Is this still something you'd like to move forward? Can you rebase and maybe address @TimothyGu's comment?

noAssert is a Boolean trap which is a terrible thing for an API with security implications. I'm all for getting rid of it. The only question is whether the performance implications are significant or not.

bnoordhuis · 2017-08-08T12:55:49Z

PTAL, significantly watered-down changeset uploaded. :-/

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

Trott · 2017-08-08T17:09:46Z

test/parallel/test-buffer-read.js

+/* eslint-disable no-restricted-syntax */
+for (const noAssert of [true, false]) {
+  // Out of bounds.
+  assert.throws(() => Buffer.from(1).readFloatBE(0, noAssert));


If I'm not mistaken, this and the subsequent lines are throwing on Buffer.from() (because it can't accept a number as an argument) and never getting to readFloatBE(). (Maybe create the Buffer external to the assert.throws() to avoid that possibility in future refactoring etc.?)

Good catch, this was hiding a real bug.

Verify that the receiver is a buffer object before attempting to write out a floating point value. Fixes: nodejs#8724

Check bounds and throw an exception when reading floating point values from a buffer. Before this commit, the process would fail a CHECK and terminate when `noAssert=true`. Fixes: nodejs#8724

Trott · 2017-08-10T02:10:44Z

@TimothyGu Does this LGTY now?

comment addressed

BridgeAR · 2017-08-26T12:25:47Z

Ping @TimothyGu

No response. Please take another look as your comment seems to be addressed.

BridgeAR · 2017-09-20T00:17:56Z

@nodejs/tsc PTAL. This is semver-major and needs further reviews.

Trott · 2017-09-20T03:04:50Z

Does this require some modifications to buffer.md to explain when noAssert is ~~deprecated and~~ ignored?

Trott · 2017-09-20T03:06:07Z

(Also: This only affects the C++ checks, but there are checks in JS-land that still respect noAssert?)

BridgeAR · 2017-09-23T00:28:05Z

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

BridgeAR · 2017-09-23T02:16:34Z

src/util-inl.h

+
+inline uint64_t Add64Clamp(uint64_t x, uint64_t y) {
+  return x + y >= x ? x + y : static_cast<uint64_t>(-1);
+}


This function seems unused.

I added it for parity (I had plans to use it for something else) but I can remove it.

I would prefer not to add functions that ~~have no functionality~~ are unused.

BridgeAR · 2017-10-02T12:03:37Z

Ping @bnoordhuis

BridgeAR · 2017-11-22T12:48:42Z

Ping @bnoordhuis

This is ready to land as soon as my comment is addressed. This could also be done while landing if you are fine with that.

Trott · 2017-11-22T12:55:15Z

This is ready to land as soon as my comment is addressed.

There's a -1 from @mscdex so this might need to be escalated to TSC if that isn't resolved.

This might need a doc update in buffer.md too so that it's documented when noAssert is ignored.

I would like to see this land.

Trott

LGTM with a doc update.

BridgeAR · 2017-11-23T00:40:04Z

@Trott thanks for pointing that out. I missed that. And good that you also caught the doc update :-)

@bnoordhuis @mscdex There will be a small performance penalty but if it truly has a 3.5 times impact, something must be wrong with V8. I think it makes sense to a) rerun the benchmark with current master and b) improve the benchmark by removing new Function. I am curious about those results. It would also be good to check the output of --prof with and without noAssert if the numbers are still weird.

Trott · 2017-11-25T14:54:03Z

I wonder if the benchmark is faulty. I can understand seeing a performance degradation with noAssert set to false, but is it plausible that we should be seeing a significant performance degradation with noAssert set to true? Here are the relevant results from a run of benchmark/buffers/buffer-read.js:

 buffers/buffer-read.js millions=1 type="DoubleBE" buffer="fast" noAssert="false"    -13.81 %        *** 2.686215e-10
 buffers/buffer-read.js millions=1 type="DoubleBE" buffer="fast" noAssert="true"     -17.24 %        *** 7.214802e-22
 buffers/buffer-read.js millions=1 type="DoubleBE" buffer="slow" noAssert="false"    -13.80 %        *** 4.725621e-14
 buffers/buffer-read.js millions=1 type="DoubleBE" buffer="slow" noAssert="true"     -15.80 %        *** 6.746154e-15
 buffers/buffer-read.js millions=1 type="DoubleLE" buffer="fast" noAssert="false"    -14.59 %        *** 2.411344e-15
 buffers/buffer-read.js millions=1 type="DoubleLE" buffer="fast" noAssert="true"     -18.60 %        *** 8.171223e-16
 buffers/buffer-read.js millions=1 type="DoubleLE" buffer="slow" noAssert="false"    -17.89 %        *** 1.790047e-19
 buffers/buffer-read.js millions=1 type="DoubleLE" buffer="slow" noAssert="true"     -16.12 %        *** 3.988442e-13
 buffers/buffer-read.js millions=1 type="FloatBE" buffer="fast" noAssert="false"     -16.16 %        *** 1.482242e-15
 buffers/buffer-read.js millions=1 type="FloatBE" buffer="fast" noAssert="true"      -15.38 %        *** 2.969582e-23
 buffers/buffer-read.js millions=1 type="FloatBE" buffer="slow" noAssert="false"     -14.22 %        *** 1.242668e-13
 buffers/buffer-read.js millions=1 type="FloatBE" buffer="slow" noAssert="true"      -17.66 %        *** 6.550393e-24
 buffers/buffer-read.js millions=1 type="FloatLE" buffer="fast" noAssert="false"     -12.73 %        *** 2.119630e-08
 buffers/buffer-read.js millions=1 type="FloatLE" buffer="fast" noAssert="true"      -16.95 %        *** 1.742243e-24
 buffers/buffer-read.js millions=1 type="FloatLE" buffer="slow" noAssert="false"     -12.40 %        *** 1.328481e-09
 buffers/buffer-read.js millions=1 type="FloatLE" buffer="slow" noAssert="true"      -18.43 %        *** 2.863973e-24

Pinging @psmarshall on the benchmark.

[Update: I think I see now why noAssert set to true would still get a performance hit although I'm kind of surprised by the size of it. Still wouldn't mind another pair of eyes on the benchmark, I guess.]

psmarshall · 2017-11-26T14:59:28Z

From experimenting locally it looks like the regression is caused by the call to Environment::GetCurrent(args). When you give that directly to the macro, it is only actually called in the error case, whereas now it is always called. If you use the version of Uint32Value() where you don't pass a Context as before, then the regression disappears. Not sure what the best solution overall is, but that seems to be where the regression comes from.

BridgeAR · 2017-12-06T00:15:55Z

Ping @bnoordhuis

BridgeAR · 2017-12-15T23:38:37Z

Ping @bnoordhuis again

bnoordhuis · 2017-12-20T02:18:51Z

I'll probably try a different tack, by moving the functions to JS. See #17775.

BridgeAR · 2017-12-28T23:39:55Z

@bnoordhuis even though #17775 got merged, I guess #8724 is still unresolved and it still makes sense to remove the noAssert option, right? Due to the changes, I guess the flag will also not have a significant impact on the performance anymore.

BridgeAR · 2018-01-12T15:22:58Z

Ping @bnoordhuis

BridgeAR · 2018-01-31T01:12:15Z

Superseded by #18395

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 Mar 29, 2017

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 29, 2017

Trott reviewed Mar 29, 2017

View reviewed changes

cjihrig approved these changes Mar 30, 2017

View reviewed changes

TimothyGu previously requested changes Mar 30, 2017

View reviewed changes

addaleax mentioned this pull request Apr 3, 2017

Type confusion bug in WriteFloatGeneric #12179

Closed

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

bnoordhuis force-pushed the buffer-remove-noassert branch from 976b0ae to 8532503 Compare August 8, 2017 12:54

Trott previously requested changes Aug 8, 2017

View reviewed changes

bnoordhuis added 2 commits August 9, 2017 18:30

src: add helpers for unsigned saturating addition

1f8c353

src: check receiver when writing floating point

8d8f4dd

Verify that the receiver is a buffer object before attempting to write out a floating point value. Fixes: nodejs#8724

bnoordhuis force-pushed the buffer-remove-noassert branch from 8532503 to 676ddbb Compare August 9, 2017 16:31

src: check bounds when reading floating point

2b3e2a2

Check bounds and throw an exception when reading floating point values from a buffer. Before this commit, the process would fail a CHECK and terminate when `noAssert=true`. Fixes: nodejs#8724

bnoordhuis force-pushed the buffer-remove-noassert branch from 676ddbb to 2b3e2a2 Compare August 9, 2017 16:37

jasnell approved these changes Sep 20, 2017

View reviewed changes

BridgeAR reviewed Sep 23, 2017

View reviewed changes

trevnorris approved these changes Oct 12, 2017

View reviewed changes

Trott approved these changes Nov 22, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR closed this Jan 31, 2018

buffer: remove noAssert argument #12119

buffer: remove noAssert argument #12119

Conversation

bnoordhuis commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 29, 2017 • edited Loading

bnoordhuis commented Mar 29, 2017

sam-github commented Mar 29, 2017

sam-github commented Mar 29, 2017

jasnell commented Mar 29, 2017

bnoordhuis commented Mar 29, 2017

cjihrig left a comment

Choose a reason for hiding this comment

jasnell commented Mar 30, 2017

Choose a reason for hiding this comment

Trott commented Aug 5, 2017

bnoordhuis commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Aug 10, 2017

BridgeAR commented Aug 26, 2017

BridgeAR commented Sep 20, 2017

Trott commented Sep 20, 2017 • edited Loading

Trott commented Sep 20, 2017

BridgeAR commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Oct 2, 2017

BridgeAR commented Nov 22, 2017

Trott commented Nov 22, 2017

Trott left a comment

Choose a reason for hiding this comment

BridgeAR commented Nov 23, 2017

Trott commented Nov 25, 2017 • edited Loading

psmarshall commented Nov 26, 2017

BridgeAR commented Dec 6, 2017

BridgeAR commented Dec 15, 2017

bnoordhuis commented Dec 20, 2017

BridgeAR commented Dec 28, 2017

BridgeAR commented Jan 12, 2018

BridgeAR commented Jan 31, 2018

mscdex commented Mar 29, 2017 •

edited

Loading

Trott commented Sep 20, 2017 •

edited

Loading

BridgeAR Sep 26, 2017 •

edited

Loading

Trott commented Nov 25, 2017 •

edited

Loading