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.

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

src: Check for overflow when resizing AliasedBufferBase #31740

Closed
wants to merge 3 commits into from
Closed

src: Check for overflow when resizing AliasedBufferBase #31740

wants to merge 3 commits into from

Conversation

amdoku
Copy link
Contributor

@amdoku amdoku commented Feb 11, 2020

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 11, 2020
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 11, 2020
@addaleax
Copy link
Member

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
Copy link
Collaborator

@amdoku
Copy link
Contributor Author

amdoku commented Feb 11, 2020

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
Copy link
Member

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

@amdoku
Copy link
Contributor Author

amdoku commented Feb 12, 2020

@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 addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 12, 2020
@addaleax
Copy link
Member

@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…

When resizing an aliased_buffer check if the new size will overflow.
@amdoku
Copy link
Contributor Author

amdoku commented Feb 14, 2020

@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.

Added native extension similar to
test/addons/stringbytes-external-exceeded-max.
Added an abort test to regression test the non overflow behaviour.
The Aliased*Array overflow check test introduced a dependency to a
compiled artifact. Add this artifact to the build process in a
similar fashion as test/addons and test/js-native-api do.
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2020
@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Mar 30, 2020
When resizing an aliased_buffer check if the new size will overflow.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
Added native extension similar to
test/addons/stringbytes-external-exceeded-max.
Added an abort test to regression test the non overflow behaviour.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
The Aliased*Array overflow check test introduced a dependency to a
compiled artifact. Add this artifact to the build process in a
similar fashion as test/addons and test/js-native-api do.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member

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

@addaleax addaleax closed this Mar 30, 2020
addaleax pushed a commit that referenced this pull request Mar 30, 2020
When resizing an aliased_buffer check if the new size will overflow.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
Added native extension similar to
test/addons/stringbytes-external-exceeded-max.
Added an abort test to regression test the non overflow behaviour.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 30, 2020
The Aliased*Array overflow check test introduced a dependency to a
compiled artifact. Add this artifact to the build process in a
similar fashion as test/addons and test/js-native-api do.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Apr 2, 2020
`cp test/addons/.gitignore test/abort/.gitignore`, because the new
addon test in there leaves a build/ folder lying around and somebody
is bound to use `git add .` earlier or later.

Refs: nodejs#31740
@richardlau
Copy link
Member

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
Copy link
Member

addaleax commented Apr 2, 2020

@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
Copy link
Member

@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

richardlau added a commit to richardlau/node-1 that referenced this pull request Apr 3, 2020
nodejs#31740 added an addon-style test to
`test/abort` that was never run because `test/abort/testcfg.py` uses the
AbortTestConfiguration which inherits from SimpleTestConfiguration which
only finds tests in the root of `test/abort`.

Make AbortTestConfiguration inherit from AddonTestConfiguration and
change AddonTestConfiguration to find the tests in the root of the test
bucket in addition to the subfolders.

Fixup `test-abort-aliased-buffer-overflow` so that it works as intended.

Signed-off-by: Richard Lau <riclau@uk.ibm.com>
addaleax added a commit that referenced this pull request Apr 5, 2020
`cp test/addons/.gitignore test/abort/.gitignore`, because the new
addon test in there leaves a build/ folder lying around and somebody
is bound to use `git add .` earlier or later.

Refs: #31740

PR-URL: #32624
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
`cp test/addons/.gitignore test/abort/.gitignore`, because the new
addon test in there leaves a build/ folder lying around and somebody
is bound to use `git add .` earlier or later.

Refs: #31740

PR-URL: #32624
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
`cp test/addons/.gitignore test/abort/.gitignore`, because the new
addon test in there leaves a build/ folder lying around and somebody
is bound to use `git add .` earlier or later.

Refs: #31740

PR-URL: #32624
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
When resizing an aliased_buffer check if the new size will overflow.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
Added native extension similar to
test/addons/stringbytes-external-exceeded-max.
Added an abort test to regression test the non overflow behaviour.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
The Aliased*Array overflow check test introduced a dependency to a
compiled artifact. Add this artifact to the build process in a
similar fashion as test/addons and test/js-native-api do.

PR-URL: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 22, 2020
`cp test/addons/.gitignore test/abort/.gitignore`, because the new
addon test in there leaves a build/ folder lying around and somebody
is bound to use `git add .` earlier or later.

Refs: #31740

PR-URL: #32624
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau added a commit to richardlau/node-1 that referenced this pull request May 6, 2020
This reverts commit babeb58.

PR-URL: nodejs#33196
Refs: nodejs#31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
richardlau added a commit to richardlau/node-1 that referenced this pull request May 6, 2020
This reverts commit e08ac09.

PR-URL: nodejs#33196
Refs: nodejs#31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request May 7, 2020
This reverts commit babeb58.

PR-URL: #33196
Refs: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request May 7, 2020
This reverts commit e08ac09.

PR-URL: #33196
Refs: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@amdoku amdoku deleted the check-for-overflow branch May 8, 2020 20:22
codebytere pushed a commit that referenced this pull request Jun 7, 2020
This reverts commit babeb58.

PR-URL: #33196
Refs: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jun 7, 2020
This reverts commit e08ac09.

PR-URL: #33196
Refs: #31740
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants