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

workflow: add test-ubsan ci #46297

Merged
merged 10 commits into from Mar 15, 2024
Merged

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Jan 20, 2023

Fixes #46062

small-icu isn't ubsan-clean, I have added it to the suppressions file.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Jan 20, 2023
common.gypi Outdated Show resolved Hide resolved
@targos targos marked this pull request as ready for review January 22, 2023 12:28
@targos targos marked this pull request as draft January 22, 2023 12:28
@targos
Copy link
Member

targos commented Jan 22, 2023

Example of an error that comes up a lot:

2023-01-22T16:07:27.6738113Z === release test-crypto-dh-leak ===
2023-01-22T16:07:27.6738530Z Path: parallel/test-crypto-dh-leak
2023-01-22T16:07:27.6746530Z ##[error]--- stderr ---
../deps/v8/src/api/api-arguments-inl.h:332:3: runtime error: call to function v8::internal::Accessors::ReconfigureToDataProperty(v8::Local<v8::Name>, v8::Local<v8::Value>, v8::PropertyCallbackInfo<v8::Boolean> const&) through pointer to incorrect function type 'void (*)(v8::Local<v8::Name>, v8::Local<v8::Value>, const v8::PropertyCallbackInfo<void> &)'
(/home/runner/work/node/node/out/Release/node+0x1e43970): note: v8::internal::Accessors::ReconfigureToDataProperty(v8::Local<v8::Name>, v8::Local<v8::Value>, v8::PropertyCallbackInfo<v8::Boolean> const&) defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../deps/v8/src/api/api-arguments-inl.h:332:3 in 
../deps/v8/src/handles/global-handles.cc:853:3: runtime error: call to function node::BaseObject::MakeWeak()::$_0::__invoke(v8::WeakCallbackInfo<node::BaseObject> const&) through pointer to incorrect function type 'void (*)(const v8::WeakCallbackInfo<void> &)'
(/home/runner/work/node/node/out/Release/node+0x1683d30): note: node::BaseObject::MakeWeak()::$_0::__invoke(v8::WeakCallbackInfo<node::BaseObject> const&) defined here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../deps/v8/src/handles/global-handles.cc:853:3 in 
node:assert:400
    throw err;
    ^

AssertionError [ERR_ASSERTION]: before=90697728 after=104456192
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-crypto-dh-leak.js:29:1)
    at Module._compile (node:internal/modules/cjs/loader:1246:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1300:10)
    at Module.load (node:internal/modules/cjs/loader:1103:32)
    at Module._load (node:internal/modules/cjs/loader:942:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

Node.js v20.0.0-pre
Command: out/Release/node --expose-gc --noconcurrent_recompilation /home/runner/work/node/node/test/parallel/test-crypto-dh-leak.js

@bnoordhuis
Copy link
Member

UBSan is technically right to complain because while V8's public type is v8::WeakCallbackInfo<T>, internally it stores and calls it as a pointer to v8::WeakCallbackInfo<void>.

I'm kind of surprised V8 doesn't have a suppression file for this; it has one for TSan but not UBSan. It's not our bug in any case so I would suggest simply suppressing it.

@RafaelGSS RafaelGSS marked this pull request as ready for review February 4, 2023 15:39
suppresions.supp Outdated Show resolved Hide resolved
@jkrems
Copy link
Contributor

jkrems commented Jul 10, 2023

@bnoordhuis It looks like the ubsan suppression for WeakCallbackInfo<T>/<void> is listed here: https://github.com/v8/v8/blob/master/tools/ubsan/ignorelist.txt#L9-L11. Or did you mean that it's missing additional places where that behavior is triggered?

@bnoordhuis
Copy link
Member

I must've overlooked that file because that's the kind of suppression I expected to see.

@RafaelGSS RafaelGSS force-pushed the build/include-ubsan branch 2 times, most recently from 1a9d4c8 to baa0456 Compare February 26, 2024 17:29
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@RafaelGSS
Copy link
Member Author

Hey @nodejs/single-executable, could you help me with the error I'm getting on test-ubsan.yml? Should I skip test-single-executable-application-empty.js when --enable-ubsan?

@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@RafaelGSS RafaelGSS added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Mar 14, 2024
@RafaelGSS RafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2024
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for setting this up!

@RafaelGSS RafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit ba06c5c into nodejs:main Mar 15, 2024
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ba06c5c

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: nodejs#46297
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #46297
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Santiago Gimeno <santiago.gimeno@gmail.com>
PR-URL: #46297
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request May 8, 2024
jkleinsc pushed a commit to electron/electron that referenced this pull request May 13, 2024
* chore: bump node in DEPS to v20.13.0

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* chore: fixup patch indices

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: handle updated filenames

- nodejs/node#51999
- nodejs/node#51927

* chore: bump node in DEPS to v20.13.1

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: update patches

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Mrnikifabio pushed a commit to Mrnikifabio/electron that referenced this pull request May 14, 2024
* chore: bump node in DEPS to v20.13.0

* crypto: enable NODE_EXTRA_CA_CERTS with BoringSSL

nodejs/node#52217

* test: skip test for dynamically linked OpenSSL

nodejs/node#52542

* lib, url: add a `windows` option to path parsing

nodejs/node#52509

* src: use dedicated routine to compile function for builtin CJS loader

nodejs/node#52016

* test: mark test as flaky

nodejs/node#52671

* build,tools: add test-ubsan ci

nodejs/node#46297

* src: preload function for Environment

nodejs/node#51539

* chore: fixup patch indices

* deps: update c-ares to 1.28.1

nodejs/node#52285

* chore: handle updated filenames

- nodejs/node#51999
- nodejs/node#51927

* chore: bump node in DEPS to v20.13.1

* events: extract addAbortListener for safe internal use

nodejs/node#52081

* module: print location of unsettled top-level await in entry points

nodejs/node#51999

* fs: add stacktrace to fs/promises

nodejs/node#49849

* chore: update patches

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
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. build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include UndefinedBehavior Sanitizer to our CI
6 participants