src: Check for overflow when resizing AliasedBufferBase

[amdoku]

When resizing an AliasedBufferBase check if the new size will overflow.

Creating a new AliasedBufferBase the constructor checks if the calculated size in bytes has overflown.
When reserving/resizing an existing buffer, the calculated new size in bytes is not checked if it has overflown.

I would want to write tests for this, but as failing the check will call node::Abort() I currently do not see how this is possible. I am open for input though.

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

[addaleax]

I would want to write tests for this, but as failing the check will call node::Abort() I currently do not see how this is possible. I am open for input though.

There are some tests that verify hard crashes in test/abort/.... Usually, the way to go about that is to spawn a child process that runs the actual test, and then assert that process’s return value and stderr.

[nodejs-github-bot]

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

[amdoku]

Had a look into test/abort/... and test/addons/stringbytes-external-exceeded-max/... to get an idea how to write aborting tests and how to write them using a native extension to get access to Aliased*Array. Should I include them in this PR or do a separate one?

[addaleax]

@amdoku Yes, ideally a regression test comes with the bugfix itself, so having it in this PR would be perfect 👍

[amdoku]

@addalex I've added the tests and made sure they pass (on linux at least).
I am confused by the exitcode and signal combination - all of the following relates to linux x86:

• abort/test-process-abort-exitcode.js calls process.abort() from javascript, which results in no exit code (exitcode === null), but with a signal SIGABRT.
• the added tests calls std::abort() from native, which results in exit code 1 and no signal.

I have found node_process_methods.cc, which I assume defines and registers the process.* javascript handles. In node_process_methods.cc#Abort the implementation calls std::abort() yet the resulting exit code and signal differ.
I would have expected those two to behave the same.

This leads me to believe that the test does not actually assert the overflow behaviour but some other error. Yet after staring and rewriting the code I can not see the error.

[addaleax]

@amdoku Yeah, this should never exit with exit code 1 (or abort() generally should never result in that). If it does, then that needs investigation…

[amdoku]

@addaleax I have fixed the problem where the test would not abort correctly.

I am very unsure about all the modifications in the build files, especially if I have added the targets in all the required places.

Sorry for not squashing the commits to be grouped more reasonable. I forgot to squash them. Fixed.

[nodejs-github-bot]

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

[addaleax]

Landed in 575b4e3...e08ac09, thanks for the PR! 🎉

[richardlau]

I missed this the first time round but the test added in the PR isn't currently being run because the test configuration for the abort tests doesn't find it.

-bash-4.2$ tools/test.py -J -p tap abort/*
TAP version 13
1..9
ok 1 abort/test-abort-backtrace
  ---
  duration_ms: 1.714
  ...
ok 2 abort/test-abort-uncaught-exception
  ---
  duration_ms: 0.214
  ...
ok 3 abort/test-addon-register-signal-handler
  ---
  duration_ms: 0.612
  ...
ok 4 abort/test-addon-uv-handle-leak
  ---
  duration_ms: 0.266
  ...
ok 5 abort/test-http-parser-consume
  ---
  duration_ms: 0.213
  ...
ok 6 abort/test-process-abort-exitcode
  ---
  duration_ms: 0.172
  ...
ok 7 abort/test-signal-handler
  ---
  duration_ms: 0.172
  ...
ok 8 abort/test-worker-abort-uncaught-exception
  ---
  duration_ms: 0.265
  ...
ok 9 abort/test-zlib-invalid-internals-usage
  ---
  duration_ms: 0.172
  ...
-bash-4.2$

We can try out the test with a quick hack (we currently don't have a test configuration that mixes addons-style tests and non-addons style tests so this hack excludes the non-addons style abort tests):

diff --git a/test/testpy/__init__.py b/test/testpy/__init__.py
index c89ab6e..8a86b1e 100644
--- a/test/testpy/__init__.py
+++ b/test/testpy/__init__.py
@@ -156,7 +156,7 @@ class AddonTestConfiguration(SimpleTestConfiguration):
             SimpleTestCase(tst, file_path, arch, mode, self.context, self, self.additional_flags))
     return result

-class AbortTestConfiguration(SimpleTestConfiguration):
+class AbortTestConfiguration(AddonTestConfiguration):
   def __init__(self, context, root, section, additional=None):
     super(AbortTestConfiguration, self).__init__(context, root, section,
                                                  additional)

which results in:

-bash-4.2$ tools/test.py -J -p tap abort/*
TAP version 13
1..1
not ok 1 abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow
  ---
  duration_ms: 0.60
  severity: fail
  exitcode: 1
  stack: |-
    internal/modules/cjs/loader.js:1010
      throw err;
      ^

    Error: Cannot find module '../common'
    Require stack:
    - /home/users/riclau/sandbox/github/nodejs/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js
        at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1007:15)
        at Function.Module._load (internal/modules/cjs/loader.js:884:27)
        at Module.require (internal/modules/cjs/loader.js:1067:19)
        at require (internal/modules/cjs/helpers.js:72:18)
        at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js:2:16)
        at Module._compile (internal/modules/cjs/loader.js:1178:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:1198:10)
        at Module.load (internal/modules/cjs/loader.js:1027:32)
        at Function.Module._load (internal/modules/cjs/loader.js:923:14)
        at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
      code: 'MODULE_NOT_FOUND',
      requireStack: [
        '/home/users/riclau/sandbox/github/nodejs/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js'
      ]
    }
  ...
-bash-4.2$

whoops, so a fixup:

diff --git a/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js b/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-o
index 33cd212..eaed311 100644
--- a/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js
+++ b/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js
@@ -1,5 +1,5 @@
 'use strict';
-const common = require('../common');
+const common = require('../../common');
 const assert = require('assert');
 const cp = require('child_process');

-bash-4.2$ tools/test.py -J -p tap abort/*
TAP version 13
1..1
not ok 1 abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow
  ---
  duration_ms: 0.173
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:102
      throw new AssertionError(obj);
      ^

    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

    1 !== null

        at ChildProcess.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/abort/test_abort-aliased-buffer-overflow/test-abort-aliased-buffer-overflow.js:24:14)
        at ChildProcess.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:362:15)
        at ChildProcess.emit (events.js:315:20)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:276:12) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: 1,
      expected: null,
      operator: 'strictEqual'
    }
  ...
-bash-4.2$

Thoughts on how to proceed? Should we revert this as it doesn't have a working test?

[addaleax]

@richardlau I wouldn’t revert the changes to src/ here, but if the test doesn’t work, I think it’s not strictly necessary that we keep it. While it’s nice to have tests, this one tests very specific internals that we don’t usually have explicit tests for.

[richardlau]

@richardlau I wouldn’t revert the changes to src/ here, but if the test doesn’t work, I think it’s not strictly necessary that we keep it. While it’s nice to have tests, this one tests very specific internals that we don’t usually have explicit tests for.

I've managed to fix up the test and get it running as part of the CI (I think, at least it works for me locally): #32626