Skip to content

workflow: add test-ubsan ci - #46297

Merged
nodejs-github-bot merged 10 commits into
nodejs:mainfrom
RafaelGSS:build/include-ubsan
Mar 15, 2024
Merged

workflow: add test-ubsan ci#46297
nodejs-github-bot merged 10 commits into
nodejs:mainfrom
RafaelGSS:build/include-ubsan

Conversation

@RafaelGSS

@RafaelGSSRafaelGSS commented Jan 20, 2023

Copy link
Copy Markdown
Member

Fixes#46062

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/gyp

@nodejs-github-botnodejs-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
Comment threadcommon.gypi Outdated
@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
Copy Markdown
Member

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
Copy Markdown
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
Comment threadsuppresions.supp Outdated
@hybrist

Copy link
Copy Markdown
Contributor

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

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

@RafaelGSS
RafaelGSSforce-pushed the build/include-ubsan branch 2 times, most recently from 1a9d4c8 to baa0456CompareFebruary 26, 2024 17:29
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
Co-Authored-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@RafaelGSS
RafaelGSSforce-pushed the build/include-ubsan branch from 2f5808c to f989175CompareMarch 2, 2024 03:04
@RafaelGSS

Copy link
Copy Markdown
MemberAuthor

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?

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@RafaelGSSRafaelGSS 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
@RafaelGSSRafaelGSS added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2024

@hybristhybrist left a comment

Copy link
Copy Markdown
Contributor

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!

@RafaelGSSRafaelGSS added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 15, 2024
@nodejs-github-botnodejs-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:mainMar 15, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in ba06c5c

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-ippolitomarco-ippolito mentioned this pull request May 2, 2024
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>
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
* chore: bump node in DEPS to v20.13.1
* chore: bump node in DEPS to v20.14.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
* 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: fixup patch indices
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2024
* chore: bump node in DEPS to v20.13.1
* chore: bump node in DEPS to v20.14.0
* chore: update build_add_gn_build_files.patch
* chore: update patches
* chore: update patches
* build: encode non-ASCII Latin1 characters as one byte in JS2C
nodejs/node#51605
* crypto: use EVP_MD_fetch and cache EVP_MD for hashes
nodejs/node#51034
* chore: update filenames.json
* chore: update patches
* src: support configurable snapshot
nodejs/node#50453
* test: remove test-domain-error-types flaky designation
nodejs/node#51717
* src: avoid draining platform tasks at FreeEnvironment
nodejs/node#51290
* chore: fix accidentally deleted v8 dep
* lib: define FormData and fetch etc. in the built-in snapshot
nodejs/node#51598
* chore: remove stray log
* 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
* deps: update c-ares to 1.28.1
nodejs/node#52285
* chore: fixup
* 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: fixup indices
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Cheng <zcbenz@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.metaIssues and PRs related to the general management of the project.needs-ciPRs 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

@RafaelGSS@nodejs-github-bot@targos@bnoordhuis@hybrist@santigimeno