GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — workflow: add test-ubsan ci by RafaelGSS · Pull Request #46297 · nodejs/node

Conversation

@RafaelGSS

@RafaelGSS RafaelGSS commented Jan 20, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Fixes #46062

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Jan 20, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/gyp
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. 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
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from 7ae3556 to 3e2e67b Compare January 20, 2023 20:26
bnoordhuis
bnoordhuis reviewed Jan 21, 2023
View reviewed changes
Comment thread common.gypi Resolved

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from 3e2e67b to 5710dcf Compare January 21, 2023 14:19
@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

targos commented Jan 22, 2023

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Jan 23, 2023

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS
RafaelGSS marked this pull request as ready for review February 4, 2023 15:39
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from 5710dcf to 25b9f77 Compare February 4, 2023 15:40
targos
targos reviewed Feb 5, 2023
View reviewed changes
Comment thread suppresions.supp Resolved

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Jul 2, 2023
Signed integer overflow in latest node 18 LTS release #48621
Closed
@hybrist

hybrist commented Jul 10, 2023

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Jul 10, 2023

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from aab758c to 1d88cb1 Compare August 12, 2023 02:48
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch 2 times, most recently from 1a9d4c8 to baa0456 Compare February 26, 2024 17:29
@RafaelGSS @santigimeno
build,tools: add test-ubsan ci
c9f3f96 
Signed-off-by: RafaelGSS <[email protected]>
Co-Authored-By: Santiago Gimeno <[email protected]>
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from baa0456 to c9f3f96 Compare February 26, 2024 17:54
RafaelGSS added 6 commits February 28, 2024 11:09
@RafaelGSS
fixup! attempt to use absolute suppressions path
682e426
@RafaelGSS
fixup! fixup! attempt to use absolute suppressions path
fc20e84
@RafaelGSS
fixup! attempt to fix suppressions
cf76754
@RafaelGSS
debug
71804c3
@RafaelGSS
fixup! debug
c803294
@RafaelGSS
workflow: adjust github env for ubsan options
f989175
@RafaelGSS
RafaelGSS force-pushed the build/include-ubsan branch from 2f5808c to f989175 Compare March 2, 2024 03:04
@RafaelGSS
fixup! workflow: adjust github env for ubsan options
f4bbe7b
@RafaelGSS
fixup! fixup! workflow: adjust github env for ubsan options
6cb6909
@RafaelGSS

RafaelGSS commented Mar 13, 2024

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS
test: skip sea tests when ubsan=1
03a8b7f
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 14, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 14, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 14, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/57747/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Mar 14, 2024
santigimeno
santigimeno approved these changes Mar 14, 2024
View reviewed changes
@RafaelGSS RafaelGSS added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 14, 2024
hybrist
hybrist approved these changes Mar 15, 2024
View reviewed changes

@hybrist hybrist left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@RafaelGSS RafaelGSS added the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 15, 2024
@nodejs-github-bot
nodejs-github-bot merged commit ba06c5c into nodejs:main Mar 15, 2024
@nodejs-github-bot

nodejs-github-bot commented Mar 15, 2024

Copy link
Copy Markdown
Collaborator

Landed in ba06c5c
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
2024-04-24, Version 22.0.0 (Current) #52505
Merged
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
@RafaelGSS @santigimeno @marco-ippolito
build,tools: add test-ubsan ci
9b277dc 
Signed-off-by: RafaelGSS <[email protected]>
Co-Authored-By: Santiago Gimeno <[email protected]>
PR-URL: #46297
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
@RafaelGSS @santigimeno @marco-ippolito
build,tools: add test-ubsan ci
93bddb5 
Signed-off-by: RafaelGSS <[email protected]>
Co-Authored-By: Santiago Gimeno <[email protected]>
PR-URL: #46297
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request May 8, 2024
@codebytere
build,tools: add test-ubsan ci
2531159 
nodejs/node#46297
jkleinsc pushed a commit to electron/electron that referenced this pull request May 13, 2024
@electron-roller @codebytere @patchup
chore: bump node to v20.13.1 (main) (#42088)
653d0f0 
* 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 <[email protected]>
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
build,tools: add test-ubsan ci
157dc11 
nodejs/node#46297
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@codebytere
build,tools: add test-ubsan ci
a58eef6 
nodejs/node#46297
codebytere added a commit to electron/electron that referenced this pull request May 31, 2024
@electron-roller @codebytere
chore: bump node to v20.14.0 (31-x-y) (#42295)
9a87938 
* 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 <[email protected]>
codebytere added a commit to electron/electron that referenced this pull request Jun 1, 2024
@electron-roller @zcbenz
@codebytere @patchup
chore: bump node to v20.14.0 (30-x-y) (#42294)
3be3381 
* 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 <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
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

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@hybrist hybrist hybrist approved these changes
@santigimeno santigimeno santigimeno approved these changes
+1 more reviewer
@targos targos targos left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. commit-queue-squash PRs the Commit Queue should land as one squashed commit. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Include UndefinedBehavior Sanitizer to our CI

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@RafaelGSS @nodejs-github-bot @targos @bnoordhuis @hybrist @santigimeno
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.