feat: impl backtrace print #1
Conversation
WalkthroughThis pull request introduces a comprehensive Rust-based segmentation fault handler for Node.js, designed to capture and report detailed backtraces when a segmentation fault occurs. The project includes cross-platform support with binaries for multiple architectures (Darwin x64/arm64, Linux x64), a robust CI/CD pipeline, and extensive configuration for development, testing, and publishing. The implementation leverages Rust's safety features and integrates with V8's stack trace capabilities to provide rich diagnostic information. Changes
Sequence DiagramsequenceDiagram
participant JS as JavaScript
participant Rust as Rust Module
participant V8 as V8 Engine
participant Signal as Signal Handler
JS->>Rust: register()
Rust->>Signal: Configure SIGSEGV handler
JS->>Rust: causeSigsegv()
Rust->>V8: Capture Stack Trace
Signal->>Rust: Trigger Segfault Handler
Rust->>Rust: Generate Backtrace
Rust-->>JS: Output Detailed Error Information
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
src/segfault_handler.rs (1)
57-64: Validate signal registration for multiple threads
If multiple threads exist, only the main thread is guaranteed to set this handler in time. For use in a multi-threaded context, confirm that each thread’s environment is properly configured or that you are comfortable with the global nature ofsigaction.src/binding.cc (1)
59-69: Consider correctness of string write operations
When calling methods likev8__String__WriteUtf8orv8__String__WriteOneByte, you may need to verify buffer sizes (including null termination requirements) to avoid writing out of bounds. Ensure that both the caller's length and the string size are consistent, and that the calling code in Rust safely checks the result.index.js (3)
1-3: Consider removing or justifying disabling lint and prettier rules.
Disabling these rules should be done with caution. If you have specific formatting or linting requirements that conflict with your build process, make sure to document them. Otherwise, consider enabling linting again to maintain code quality.
306-311: Improve error handling or logging to identify native binding loading issues.
CatchingloadErroris good, but a more descriptive log message showing the attempted binding paths could help diagnose environment-specific load failures.
313-316: Provide usage detail for exported functions.
ExposingcauseSigsegvandregisteris clear, but consider in-code comments or docstrings on how to properly use them, especially to highlight thatcauseSigsegvintentionally crashes the process.index.d.ts (1)
6-7: Add doc comments for the declared exports.
Providing doc comments helps teams and consumers quickly understand thatregister()sets up the segfault handler, andcauseSigsegv()intentionally crashes the process.src/lib.rs (1)
9-15: Add documentation for the test-only function.This function intentionally causes a segmentation fault and should be clearly marked as test-only with appropriate documentation.
Add documentation explaining the purpose and risks:
#[napi] +/// Causes a segmentation fault by dereferencing a null pointer. +/// +/// # Safety +/// This function is intended for testing purposes only. +/// It will crash the process by causing a segmentation fault. +#[cfg(test)] pub fn cause_sigsegv() {build.rs (1)
20-26: Add platform-specific compilation flags.The C++ build configuration should handle platform-specific requirements.
Add platform-specific flags:
cc::Build::new() .cpp(true) .flag_if_supported("-std=c++17") .flag("-Wno-unused-parameter") + .flag_if_supported(if cfg!(target_os = "macos") { + "-mmacosx-version-min=10.13" + } else { + "" + }) + .flag_if_supported(if cfg!(target_env = "msvc") { + "/EHsc" + } else { + "" + }) .include(node_include_dir) .file("src/binding.cc") .compile("extend_v8_binding");README.md (3)
15-61: Consider sanitizing file paths in example output.The example backtraces contain absolute file paths that expose your local directory structure (e.g.,
/Users/killa/). Consider replacing these with generic paths for documentation.-v8 backtrace start - <anonymous> (/Users/killa/workspace/projects/node-segfault-handler-rs/node-segfault-handler-rs/test/fixtures/test.js:6:3) +v8 backtrace start + <anonymous> (/path/to/project/test/fixtures/test.js:6:3)
63-71: Consider adding prerequisite information.The developer section could benefit from additional details:
- Required Rust version
- Required Node.js version
- Any platform-specific requirements
73-76: LGTM! Consider a minor wording refinement.The acknowledgments are appropriate and give credit where due. Consider using more formal language:
-Thanks to the [napi-rs](https://github.com/napi-rs/napi-rs) for delivering an amazing Rust development experience. +Thanks to the [napi-rs](https://github.com/napi-rs/napi-rs) for delivering an excellent Rust development experience.🧰 Tools
🪛 LanguageTool
[style] ~76-~76: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...hub.com/napi-rs/napi-rs) for delivering an amazing Rust development experience.(AWESOME)
.github/workflows/CI.yml (1)
216-226: Consider enhancing version validation.The version checking logic could be more robust. Consider using semantic version regex for stricter validation:
- if git log -1 --pretty=%B | grep "^[0-9]\+\.[0-9]\+\.[0-9]\+$"; + if git log -1 --pretty=%B | grep -E "^(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
.cargo/config.toml(1 hunks).eslintignore(1 hunks).eslintrc(1 hunks).github/workflows/CI.yml(1 hunks).gitignore(1 hunks).npmignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)build.rs(1 hunks)index.d.ts(1 hunks)index.js(1 hunks)npm/darwin-arm64/README.md(1 hunks)npm/darwin-arm64/package.json(1 hunks)npm/darwin-universal/README.md(1 hunks)npm/darwin-universal/package.json(1 hunks)npm/darwin-x64/README.md(1 hunks)npm/darwin-x64/package.json(1 hunks)npm/linux-x64-gnu/README.md(1 hunks)npm/linux-x64-gnu/package.json(1 hunks)npm/linux-x64-musl/README.md(1 hunks)npm/linux-x64-musl/package.json(1 hunks)package.json(1 hunks)rustfmt.toml(1 hunks)src/binding.cc(1 hunks)src/binding.rs(1 hunks)src/lib.rs(1 hunks)src/segfault_handler.rs(1 hunks)test/fixtures/test.js(1 hunks)test/index.test.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (18)
- npm/darwin-universal/README.md
- npm/linux-x64-gnu/README.md
- tsconfig.json
- rustfmt.toml
- .eslintignore
- npm/darwin-x64/README.md
- npm/linux-x64-musl/package.json
- .npmignore
- npm/linux-x64-gnu/package.json
- .eslintrc
- npm/linux-x64-musl/README.md
- npm/darwin-arm64/README.md
- npm/darwin-arm64/package.json
- npm/darwin-x64/package.json
- npm/darwin-universal/package.json
- .gitignore
- package.json
- Cargo.toml
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~76-~76: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...hub.com/napi-rs/napi-rs) for delivering an amazing Rust development experience.
(AWESOME)
🪛 actionlint (1.7.4)
.github/workflows/CI.yml
73-73: property "setup" is not defined in object type {build: string; docker: string; host: string; target: string}
(expression)
74-74: property "setup" is not defined in object type {build: string; docker: string; host: string; target: string}
(expression)
🔇 Additional comments (12)
src/segfault_handler.rs (2)
1-8: Consider the signal-safety of imported functions
Imports such asbacktraceandbacktrace_symbols_fdcan be used carefully in a signal handler context, but you should confirm that linking (vianix::libc) and using them withinsigactionis fully supported on all target platforms. In most cases, these calls are acceptable here, but it is worth noting that fully async-signal-safe usage is quite restricted in POSIX.
30-55: Handle potential V8 isolate unavailability
When callingIsolate::get_isolate(), it may returnNoneif the isolate is unavailable (for example, if the segfault happened before the isolate was correctly initialized). Consider gracefully handling aNonecase to avoid dereferencing a null pointer in a compromised environment.src/binding.cc (1)
20-23: Validate object lifetimes in returned pointers
All functions returning raw pointers to V8 objects (StackFrames, Strings, etc.) rely on the assumption that the isolate and the underlying objects remain valid for as long as the pointers are used. Ensure the Rust side calls them immediately or retains them only within a known-safe scope.Also applies to: 25-27, 29-33, 35-38, 40-42, 45-47, 49-51, 53-53
src/binding.rs (3)
1-8: Confirm that bitflags and FFI types align with C side
You’re definingbitflagsin Rust and referencing them in your C++ code. Confirm that the numeric values/configuration ofWriteOptionsmatch exactly what the C++ side expects. Also confirmOpaque = [u8; 0]usage is consistent with how the library handles memory for each type.
25-27: Ensure the correct linkage with “extend_v8_binding”
The#[link(name = "extend_v8_binding")]attribute relies on the correct library name and link path at compile time. Confirm that your build system places and names the library consistently across all supported platforms.
78-92: Check conversion toframe_limit
frame_limit.try_into()could fail ifframe_limitexceedsc_int::MAX. The code currently discards the error. Consider graceful handling or a fallback.index.js (1)
16-29: ValidateisMusldetection approach.
Parsing the output ofwhich lddmay fail on distributions without a conventional environment or on restricted containers. Consider an alternative check, or ensure that missing commands are gracefully handled.test/fixtures/test.js (1)
1-7: Ensure controlled test environment for segfault.
In a CI or automated setting, triggering a segfault will crash the process. Verify your testing framework can handle or capture such crashes to avoid mistakenly marking the entire test suite as failed.README.md (1)
1-13: LGTM! Clear and concise installation and usage instructions..github/workflows/CI.yml (3)
1-21: LGTM! Well-configured workflow triggers and permissions.The workflow configuration follows security best practices with appropriate permissions and path exclusions.
95-155: LGTM! Comprehensive test strategy across platforms.Good coverage of different Node.js versions and Linux variants using isolated Docker environments.
156-187: LGTM! Well-implemented universal binary build.The universal macOS binary build process is properly configured with appropriate error handling.
src/segfault_handler.rs
Outdated
| extern "C" fn segfault_action(_sig: c_int, _si: *mut nix::libc::siginfo_t, _: *mut c_void) { | ||
| { | ||
| eprintln!("rust backtrace start"); | ||
| let backtrace = Backtrace::force_capture(); | ||
| eprintln!("{:#?}", backtrace); | ||
| eprintln!("rust backtrace end"); | ||
| } | ||
|
|
||
| unsafe { | ||
| eprintln!("c backtrace start"); | ||
| let mut array: [*mut c_void; STACK_LIMIT] = [std::ptr::null_mut(); STACK_LIMIT]; | ||
| let size = backtrace(array.as_mut_ptr(), STACK_LIMIT as c_int); | ||
| backtrace_symbols_fd(array.as_ptr(), size, 2); | ||
| eprintln!("c backtrace end"); | ||
| } | ||
|
|
||
| eprintln!("v8 backtrace start"); | ||
| print_v8_backtrace(); | ||
| eprintln!("v8 backtrace end"); | ||
| } |
There was a problem hiding this comment.
Avoid calling complex I/O and memory-allocating operations inside a signal handler
Functions like eprintln!, the Rust Backtrace::force_capture(), and even the C backtrace_symbols_fd can allocate, grab locks, or do other non-async-signal-safe operations. If execution continues in an undefined or unsafe state, these calls could lead to deadlocks or re-entrant crashes. Although common for debugging, note that strictly conforming to async-signal-safe guidelines typically limits you to minimal actions such as calling low-level, reentrant syscalls.
| template <class T> | ||
| inline static T* local_to_ptr(v8::Local<T> local) { | ||
| return *local; | ||
| } |
There was a problem hiding this comment.
Check for empty handles before dereferencing
local_to_ptr(v8::Local<T> local) returns *local without verifying that the handle is not empty. If the local handle is empty, this would dereference a null pointer. Consider adding a runtime check or an assertion.
template <class T>
inline static T* local_to_ptr(v8::Local<T> local) {
- return *local;
+ if (local.IsEmpty()) {
+ return nullptr; // or handle the error more gracefully
+ }
+ return *local;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <class T> | |
| inline static T* local_to_ptr(v8::Local<T> local) { | |
| return *local; | |
| } | |
| template <class T> | |
| inline static T* local_to_ptr(v8::Local<T> local) { | |
| if (local.IsEmpty()) { | |
| return nullptr; // or handle the error more gracefully | |
| } | |
| return *local; | |
| } |
| impl Isolate { | ||
| pub fn get_isolate() -> Option<&'static mut Isolate> { | ||
| unsafe { v8__GetIsolate().as_mut() } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard usage of get_isolate()
pub fn get_isolate() may return None. This is valid Rust but can lead to crashes if the caller makes assumptions about isolate availability (for example, due to uninitialized V8 in the current thread). Consider logging or returning a Result instead for improved safety.
| #[inline(always)] | ||
| pub fn write_one_byte_uninit( | ||
| &self, | ||
| scope: &Isolate, | ||
| buffer: &mut [MaybeUninit<u8>], | ||
| start: usize, | ||
| options: WriteOptions, | ||
| ) -> usize { | ||
| unsafe { | ||
| v8__String__WriteOneByte( | ||
| self, | ||
| scope, | ||
| buffer.as_mut_ptr() as *mut u8, | ||
| start.try_into().unwrap_or(c_int::MAX), | ||
| buffer.len().try_into().unwrap_or(c_int::MAX), | ||
| options, | ||
| ) as usize | ||
| } | ||
| } |
There was a problem hiding this comment.
Review MaybeUninit usage for string writes
Using MaybeUninit for buffer creation can be tricky. If v8__String__WriteUtf8 or v8__String__WriteOneByte writes fewer bytes than expected, you’re safe, but if it writes more, you could corrupt memory. Validate the result and handle partial writes or mismatches carefully.
Also applies to: 169-191
| if len_utf16 == 0 { | ||
| return std::string::String::new(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Release memory or reuse buffers for strings
Manually allocating with std::alloc::alloc and converting to a String via from_raw_parts is powerful but risky. Each newly created string must be carefully freed or allowed to be dropped by Rust. Check for potential memory leaks (e.g. if the code panics midway) and ensure no re-use of the same pointer across calls.
Also applies to: 201-205, 207-226, 228-249
src/lib.rs
Outdated
| #[napi] | ||
| pub fn register() -> () { | ||
| segfault_handler::segfault_handler(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation for the register function.
The registration function should handle potential errors and provide clear documentation about its purpose and behavior.
Add error handling and documentation:
#[napi]
+/// Registers a segmentation fault handler that captures and prints backtraces.
+///
+/// # Returns
+/// Result indicating whether the handler was successfully registered.
+///
+/// # Examples
+/// ```no_run
+/// register().expect("Failed to register segfault handler");
+/// ```
-pub fn register() -> () {
- segfault_handler::segfault_handler();
+pub fn register() -> napi::Result<()> {
+ segfault_handler::segfault_handler().map_err(|e| {
+ napi::Error::from_reason(format!("Failed to register segfault handler: {}", e))
+ })
}| let node_include_dir = if let Ok(node_dir) = std::env::var("NODE_DIR") { | ||
| PathBuf::from(node_dir).join("include/node") | ||
| } else if let Ok(node_exec_path) = which::which("node") { | ||
| if let Some(node_bin_dir) = node_exec_path.parent() { | ||
| node_bin_dir.join("../include/node") | ||
| } else { | ||
| panic!("node install with out include dir") | ||
| } | ||
| } else { | ||
| panic!("not found node exec, please set NODE_DIR env like ~/.nvm/versions/node/v20.17.0"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error messages and path validation.
The Node.js path resolution could be more robust with better error messages and path validation.
Consider these improvements:
let node_include_dir = if let Ok(node_dir) = std::env::var("NODE_DIR") {
- PathBuf::from(node_dir).join("include/node")
+ let path = PathBuf::from(node_dir).join("include/node");
+ if !path.exists() {
+ panic!("Node.js include directory not found at: {}", path.display());
+ }
+ path
} else if let Ok(node_exec_path) = which::which("node") {
if let Some(node_bin_dir) = node_exec_path.parent() {
- node_bin_dir.join("../include/node")
+ let path = node_bin_dir.join("../include/node");
+ if !path.exists() {
+ panic!("Node.js include directory not found at: {}", path.display());
+ }
+ path
} else {
- panic!("node install with out include dir")
+ panic!("Invalid Node.js installation: executable has no parent directory")
}
} else {
- panic!("not found node exec, please set NODE_DIR env like ~/.nvm/versions/node/v20.17.0");
+ panic!("Node.js executable not found. Please ensure Node.js is installed or set NODE_DIR environment variable (e.g., ~/.nvm/versions/node/v20.17.0)");
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let node_include_dir = if let Ok(node_dir) = std::env::var("NODE_DIR") { | |
| PathBuf::from(node_dir).join("include/node") | |
| } else if let Ok(node_exec_path) = which::which("node") { | |
| if let Some(node_bin_dir) = node_exec_path.parent() { | |
| node_bin_dir.join("../include/node") | |
| } else { | |
| panic!("node install with out include dir") | |
| } | |
| } else { | |
| panic!("not found node exec, please set NODE_DIR env like ~/.nvm/versions/node/v20.17.0"); | |
| }; | |
| let node_include_dir = if let Ok(node_dir) = std::env::var("NODE_DIR") { | |
| let path = PathBuf::from(node_dir).join("include/node"); | |
| if !path.exists() { | |
| panic!("Node.js include directory not found at: {}", path.display()); | |
| } | |
| path | |
| } else if let Ok(node_exec_path) = which::which("node") { | |
| if let Some(node_bin_dir) = node_exec_path.parent() { | |
| let path = node_bin_dir.join("../include/node"); | |
| if !path.exists() { | |
| panic!("Node.js include directory not found at: {}", path.display()); | |
| } | |
| path | |
| } else { | |
| panic!("Invalid Node.js installation: executable has no parent directory") | |
| } | |
| } else { | |
| panic!("Node.js executable not found. Please ensure Node.js is installed or set NODE_DIR environment variable (e.g., ~/.nvm/versions/node/v20.17.0)"); | |
| }; |
test/index.test.ts
Outdated
| describe('test/index.test.ts', () => { | ||
| it('should print stack to stdout', async () => { | ||
| const res = await coffee.fork(path.join(__dirname, 'fixtures/test.js')) | ||
| // FIXME coffee exit code is null | ||
| // .expect('code', 139) | ||
| .end(); | ||
| assert(res.stderr.match(/test\/fixtures\/test\.js:6:3/)); | ||
| assert(res.stderr.match(/listOnTimeout/)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and fix exit code verification.
The test suite needs improvements in several areas:
- The exit code verification is commented out
- Limited assertion coverage
- Missing error cases
Consider these improvements:
describe('test/index.test.ts', () => {
+ it('should successfully register handler', async () => {
+ const res = await coffee.fork(path.join(__dirname, 'fixtures/register.js'))
+ .expect('code', 0)
+ .end();
+ assert(res.stderr === '');
+ });
+
it('should print stack to stdout', async () => {
const res = await coffee.fork(path.join(__dirname, 'fixtures/test.js'))
- // FIXME coffee exit code is null
- // .expect('code', 139)
+ .expect('code', process.platform === 'linux' ? 139 : 134) // SIGSEGV
.end();
assert(res.stderr.match(/test\/fixtures\/test\.js:6:3/));
assert(res.stderr.match(/listOnTimeout/));
+ assert(res.stderr.includes('SIGSEGV'), 'Should indicate segmentation fault');
});
+
+ it('should handle registration errors', async () => {
+ const res = await coffee.fork(path.join(__dirname, 'fixtures/double_register.js'))
+ .expect('code', 1)
+ .end();
+ assert(res.stderr.includes('Failed to register'));
+ });
});Committable suggestion skipped: line range outside the PR's diff.
.cargo/config.toml
Outdated
| [target.aarch64-unknown-linux-musl] | ||
| linker = "aarch64-linux-musl-gcc" | ||
| rustflags = ["-C", "target-feature=-crt-static"] |
There was a problem hiding this comment.
💡 Codebase verification
Document cross-compiler requirement and add installation instructions
The configuration requires aarch64-linux-musl-gcc for cross-compilation, but the linker is not available in the build environment. Add documentation to help developers set up the required toolchain:
+# Cross-compilation configuration for aarch64 Linux with musl libc
+# Requires: aarch64-linux-musl-gcc (musl cross-compiler)
+# Installation:
+# - Debian/Ubuntu: apt install musl-tools gcc-aarch64-linux-gnu
+# - Alpine: apk add musl-dev gcc-aarch64-linux-musl
+# - Other: https://musl.cc/
+#
[target.aarch64-unknown-linux-musl]
linker = "aarch64-linux-musl-gcc"
rustflags = ["-C", "target-feature=-crt-static"]🔗 Analysis chain
Add documentation and verify linker availability.
The target configuration needs documentation and runtime verification of the linker.
Add a comment explaining the configuration:
+# Configuration for building on aarch64 Linux with musl libc
+# Disables static linking of the C runtime to ensure compatibility
+# with Node.js native modules
[target.aarch64-unknown-linux-musl]
linker = "aarch64-linux-musl-gcc"
rustflags = ["-C", "target-feature=-crt-static"]Add a script to verify the linker availability:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the required linker is available for aarch64 builds
if [ "$(uname -m)" = "aarch64" ]; then
if ! command -v aarch64-linux-musl-gcc &> /dev/null; then
echo "Error: aarch64-linux-musl-gcc is required but not found"
echo "Please install the appropriate cross-compilation toolchain"
exit 1
fi
fi
Length of output: 38
Script:
#!/bin/bash
# Check for cross-compiler availability
command -v aarch64-linux-musl-gcc
echo $?
# Check if it's properly configured
if command -v aarch64-linux-musl-gcc >/dev/null 2>&1; then
aarch64-linux-musl-gcc --version
fi
# Look for any related cross-compilation setup in package files
fd -t f "cross|toolchain" -e json -e toml -e yaml
Length of output: 134
.github/workflows/CI.yml
Outdated
| run: ${{ matrix.settings.setup }} | ||
| if: ${{ matrix.settings.setup }} | ||
| shell: bash |
There was a problem hiding this comment.
Fix undefined matrix property usage.
The setup property is referenced in the workflow but not defined in any of the matrix configurations. This could cause the workflow to fail.
Either:
- Remove these steps if they're not needed:
- - name: Setup toolchain
- run: ${{ matrix.settings.setup }}
- if: ${{ matrix.settings.setup }}
- shell: bash- Or add the
setupproperty to the matrix configurations where needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: ${{ matrix.settings.setup }} | |
| if: ${{ matrix.settings.setup }} | |
| shell: bash | |
| shell: bash |
🧰 Tools
🪛 actionlint (1.7.4)
73-73: property "setup" is not defined in object type {build: string; docker: string; host: string; target: string}
(expression)
74-74: property "setup" is not defined in object type {build: string; docker: string; host: string; target: string}
(expression)
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
package.json (2)
29-40: Script Commands Configuration
The scripts comprehensively cover linting, building (both release and debug), prepublish tasks, and testing. As a minor nitpick, consider adding a "start" script if there is a runtime entry point or if you plan to run the built module directly.
41-42: Repository Information
The repository URL is specified using the SSH format. If you intend for the repository to be publicly accessible in a read-only mode, consider switching to an HTTPS URL to improve accessibility for external collaborators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
.github/workflows/CI.yml(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/CI.yml
🔇 Additional comments (5)
package.json (5)
1-5: Basic Metadata Fields
The basic project metadata (name, version, main, and types) is clearly defined. Note that the version is currently set to "0.0.0", which is acceptable for an initial commit but should be updated appropriately as development progresses.
6-15: N-API Binding Configuration
The "napi" section is well-structured and defines both the package name and the supported target triples. This configuration aligns with the objectives of supporting multiple architectures. Please verify that the specified triples ("aarch64-apple-darwin", "x86_64-unknown-linux-gnu", and "universal-apple-darwin") fully match your intended target environments and CI/CD setup.
16-16: License Declaration
The use of the MIT license is appropriate for an open-source Node.js project.
17-25: DevDependencies Setup
The devDependencies provide a solid foundation for TypeScript configuration, CLI tooling, linting, and testing. Ensure that all these tools are compatible with Node.js 20 as specified in the engines field. If you are not using CoffeeScript actively, you might consider reviewing its necessity; however, if it's part of your build/test process, it’s fine.
26-28: Engine Requirement
The engines field restricts the project to Node.js version 20 or higher, which is consistent with leveraging modern Node features.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/binding.cc (1)
31-34: Ensure isolate existence
v8::Isolate::GetCurrent()can returnnullptrif there is no current isolate on this thread. Downstream code should handle anullptrscenario or ensure that an isolate is always set prior to calling this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/binding.cc(1 hunks)src/binding.rs(1 hunks)src/segfault_handler.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/segfault_handler.rs
- package.json
🔇 Additional comments (5)
src/binding.cc (2)
26-29: Check for empty handles before dereferencing
This function dereferences*localwithout verifying that the handle is not empty. If the local handle is empty, this would dereference a null pointer.
36-88: Overall implementation looks good
These binding functions appear consistent with V8’s recommended usage. No major issues found.src/binding.rs (3)
78-82: Guard usage ofget_isolate()
ReturningOptionis good, but you may still want to log or handle the case whereget_isolate()returnsNoneif V8 is uninitialized in the current thread.
157-175: ReviewMaybeUninitusage for string writes
UsingMaybeUninitfor buffer creation can be risky ifv8__String__WriteOneBytewrites more bytes than the buffer size. Validate the result and handle partial writes carefully.
205-207: Release or reuse allocated buffers for strings
Manually allocating memory and converting to aStringcan lead to leaks if the function panics. Ensure correct handling of allocated memory and that pointers aren’t reused.
| if self.is_onebyte() && len_utf8 == len_utf16 { | ||
| unsafe { | ||
| // Create an uninitialized buffer of `capacity` bytes. We need to be careful here to avoid | ||
| // accidentally creating a slice of u8 which would be invalid. | ||
| let layout = std::alloc::Layout::from_size_align(len_utf16, 1).unwrap(); | ||
| let data = std::alloc::alloc(layout) as *mut MaybeUninit<u8>; | ||
| let buffer = std::ptr::slice_from_raw_parts_mut(data, len_utf16); | ||
|
|
||
| // Write to this MaybeUninit buffer, assuming we're going to fill this entire buffer | ||
| let length = self.write_one_byte_uninit( | ||
| scope, | ||
| &mut *buffer, | ||
| 0, | ||
| WriteOptions::NO_NULL_TERMINATION | WriteOptions::REPLACE_INVALID_UTF8, | ||
| ); | ||
| debug_assert!(length == len_utf16); | ||
|
|
||
| // Return an owned string from this guaranteed now-initialized data | ||
| let buffer = data as *mut u8; | ||
| return std::string::String::from_raw_parts(buffer, length, len_utf16); | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: This allocates a buffer manually using the default allocator using the string's capacity. | ||
| // We have a large number of invariants to uphold, so please check changes to this code carefully | ||
| unsafe { | ||
| // Create an uninitialized buffer of `capacity` bytes. We need to be careful here to avoid | ||
| // accidentally creating a slice of u8 which would be invalid. | ||
| let layout = std::alloc::Layout::from_size_align(len_utf8, 1).unwrap(); | ||
| let data = std::alloc::alloc(layout) as *mut MaybeUninit<u8>; | ||
| let buffer = std::ptr::slice_from_raw_parts_mut(data, len_utf8); | ||
|
|
||
| // Write to this MaybeUninit buffer, assuming we're going to fill this entire buffer | ||
| let length = self.write_utf8_uninit( | ||
| scope, | ||
| &mut *buffer, | ||
| None, | ||
| WriteOptions::NO_NULL_TERMINATION | WriteOptions::REPLACE_INVALID_UTF8, | ||
| ); | ||
| debug_assert!(length == len_utf8); | ||
|
|
||
| // Return an owned string from this guaranteed now-initialized data | ||
| let buffer = data as *mut u8; | ||
| std::string::String::from_raw_parts(buffer, length, len_utf8) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Subject: Insufficient Runtime Handling for Partial Writes in to_rust_string_lossy
The investigation confirms that the function relies solely on debug_assert! to verify that the entire buffer has been written. In non‑debug builds the assertions are omitted, which means that any unexpected partial write (for example, when invalid UTF‑8 is replaced) could lead to memory corruption or incomplete strings without detectable errors at runtime.
src/binding.rs(Lines 213–258): The code usesdebug_assert!(length == len_utf16)anddebug_assert!(length == len_utf8)to confirm full writes, but these checks are stripped in release builds.- Call Sites: Searches (e.g., in
src/segfault_handler.rs) show that callers ofto_rust_string_lossydo not implement additional handling for partial writes.
It is recommended to add runtime checks to gracefully handle scenarios where writing does not produce the expected length, rather than relying on debug_assert! alone.
🔗 Analysis chain
Verify partial write handling
You rely on debug_assert!(length == len_utf16) or debug_assert!(length == len_utf8) to confirm complete writes. Any mismatch can corrupt memory or produce incomplete strings if invalid UTF-8 is replaced. Consider gracefully handling unexpected partial writes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Purpose: Find code calling `to_rust_string_lossy` and determine if partial writes are handled.
fd --type f --extension rs | xargs rg 'to_rust_string_lossy' -A 5
Length of output: 1063
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/segfault_handler.rs (2)
41-66: Avoid calling into V8 from the signal handler.Invoking
print_v8_backtrace(), which interacts with the JavaScript runtime, is similarly not guaranteed to be safe after a segmentation fault. If the V8 isolate is in an undefined state, these calls may lead to more severe or misleading crashes. Consider deferring these calls to a separate diagnostic phase if reliability is paramount.
56-56: Remove commented-out code for clarity.The commented-out line “
// script_name.unwrap()” appears stale or unused. Consider removing it to maintain code cleanliness and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/CI.yml(1 hunks)src/lib.rs(1 hunks)src/segfault_handler.rs(1 hunks)test/index.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/index.test.ts
- src/lib.rs
- .github/workflows/CI.yml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish
- GitHub Check: Test bindings on macos - node@22
- GitHub Check: Test bindings on macos - node@20
🔇 Additional comments (2)
src/segfault_handler.rs (2)
12-39: Beware of non-async-signal-safe operations within the signal handler.Calls like
eprintln!(), memory allocation viaBacktrace::force_capture(), or the symbol resolution functions are not strictly async-signal-safe and can result in deadlocks or re-entrant crashes if the process state is compromised. This pattern is often acceptable for debugging, but be aware of the risks in production or critical environments.
68-75: Verify signal-handler chaining and error behavior.Registering a custom SIGSEGV handler overrides any existing handler. Ensure this is intentional and consider whether you need to chain to previous handlers. Also confirm the desired behavior after this handler completes, as
SA_RESETHANDautomatically restores default handling for subsequent faults.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
README.md (2)
15-61: Comprehensive Backtrace Logging Samples:
The "Log" section provides an extensive sample of backtrace outputs covering Rust, C, and v8. This is very useful for debugging segmentation faults. To improve clarity for users, consider adding a brief note (e.g., immediately after the "## Log" header) stating that the output is an example and actual log details may vary based on the system configuration and runtime environment.
73-76: Language Style in Acknowledgements:
The acknowledgements section credits the inspirations for the project effectively. However, the phrase “delivering an amazing Rust development experience” can be made more formal. For example, replacing "amazing" with "excellent" would help maintain a professional tone.- - Thanks to the [napi-rs](https://github.com/napi-rs/napi-rs) for delivering an amazing Rust development experience. + - Thanks to the [napi-rs](https://github.com/napi-rs/napi-rs) for delivering an excellent Rust development experience.🧰 Tools
🪛 LanguageTool
[style] ~76-~76: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...hub.com/napi-rs/napi-rs) for delivering an amazing Rust development experience.(AWESOME)
index.js (2)
31-304: Improve error handling with detailed messages.The current error handling only captures the error object but doesn't provide context about which binding failed to load.
Apply this pattern throughout the try-catch blocks:
} catch (e) { - loadError = e + loadError = new Error( + `Failed to load native binding for ${platform}-${arch}: ${e.message}`, + { cause: e } + ) }
306-311: Enhance error messages with platform details.When native binding fails to load, include platform and architecture information in the error message.
Apply this diff:
if (loadError) { throw loadError } - throw new Error(`Failed to load native binding`) + throw new Error(`Failed to load native binding for ${platform}-${arch}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.eslintignore(1 hunks).eslintrc(1 hunks).github/workflows/CI.yml(1 hunks).gitignore(1 hunks).npmignore(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)build.rs(1 hunks)index.d.ts(1 hunks)index.js(1 hunks)npm/darwin-arm64/README.md(1 hunks)npm/darwin-arm64/package.json(1 hunks)npm/darwin-universal/README.md(1 hunks)npm/darwin-universal/package.json(1 hunks)npm/darwin-x64/README.md(1 hunks)npm/darwin-x64/package.json(1 hunks)npm/linux-x64-gnu/README.md(1 hunks)npm/linux-x64-gnu/package.json(1 hunks)package.json(1 hunks)rustfmt.toml(1 hunks)src/binding.cc(1 hunks)src/binding.rs(1 hunks)src/lib.rs(1 hunks)src/segfault_handler.rs(1 hunks)test/fixtures/test.js(1 hunks)test/index.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- npm/linux-x64-gnu/README.md
- npm/darwin-universal/README.md
- npm/darwin-arm64/README.md
- .eslintrc
- npm/darwin-x64/README.md
- rustfmt.toml
- .eslintignore
- index.d.ts
- test/fixtures/test.js
- npm/darwin-universal/package.json
- tsconfig.json
- npm/darwin-x64/package.json
- npm/darwin-arm64/package.json
- .npmignore
- test/index.test.ts
- npm/linux-x64-gnu/package.json
- build.rs
- src/segfault_handler.rs
- .github/workflows/CI.yml
- .gitignore
- package.json
- Cargo.toml
- src/lib.rs
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~76-~76: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...hub.com/napi-rs/napi-rs) for delivering an amazing Rust development experience.
(AWESOME)
🔇 Additional comments (11)
src/binding.cc (1)
27-29: Check for empty handles before dereferencingThis code snippet directly dereferences the V8 local handle without verifying that it is not empty, potentially causing a null pointer dereference. A similar point was raised previously and remains unaddressed. Consider adding a check or assertion to avoid undefined behavior.
src/binding.rs (3)
79-82: Further safeguard isolate availabilityReturning an
Optionis a step in the right direction, but previous feedback suggested also logging or returning aResultto handle cases where the isolate is unavailable. This can prevent silent failures and make error handling more explicit.
157-199: Validate potential buffer overflows and partial writes withMaybeUninitAs previously discussed, the usage of
MaybeUninitfor string writes risks undefined behavior if the write functions overrun the buffer capacity or if returns are not strictly validated. Ensuring a robust check for written length in both debug and release modes would safeguard against corruption.
201-259: Runtime checks needed for partial writes and memory managementThis logic relies on
debug_assertto verify expected lengths, but these assertions are omitted in release mode. A prior review highlighted the need for runtime checks to prevent memory corruption or truncated strings. Also consider whether additional safeguards are needed if a panic occurs before the newly allocated memory is fully initialized or consumed.README.md (4)
1-2: Clear "Quick Start" Header:
The "Quick Start" header immediately tells the reader where to begin. This straightforward approach is helpful for new users.
3-7: Installation Instructions Clarity:
The installation instructions using the shell commandnpm i --save node-segfault-handler-rsare concise and clear. While using "npm i" is common, you might consider mentioning that "npm install" is an equally acceptable alternative for clarity.
9-13: Usage Example Clarity:
The JavaScript snippet that shows how to require and invoke the segfault handler (viaregister()) is well presented. This clear example aids users in understanding how to integrate the package.
63-72: Developer Instructions for Build and Test:
The "For Developer" section clearly specifies the steps to build and test the project when modifying Rust code (npm run buildfollowed bynpm run test). This is helpful for ensuring that changes are correctly validated.index.js (3)
1-14: LGTM!The imports and variable declarations are well-structured and follow Node.js conventions.
31-304: Large switch block can be simplified for maintainability.Loading platform-specific bindings with a massive switch statement is prone to duplication and is difficult to maintain.
313-316: LGTM!The exports are clean and follow Node.js conventions.
| function isMusl() { | ||
| // For Node 10 | ||
| if (!process.report || typeof process.report.getReport !== 'function') { | ||
| try { | ||
| const lddPath = require('child_process').execSync('which ldd').toString().trim() | ||
| return readFileSync(lddPath, 'utf8').includes('musl') | ||
| } catch (e) { | ||
| return true | ||
| } | ||
| } else { | ||
| const { glibcVersionRuntime } = process.report.getReport().header | ||
| return !glibcVersionRuntime | ||
| } | ||
| } |
There was a problem hiding this comment.
Security concern in musl detection for Node 10.
The which ldd command execution could be dangerous if PATH is compromised. Consider:
- Using an absolute path to
ldd - Implementing additional validation before executing the command
- Adding a warning log when falling back to true on error
Apply this diff to improve security:
function isMusl() {
// For Node 10
if (!process.report || typeof process.report.getReport !== 'function') {
try {
- const lddPath = require('child_process').execSync('which ldd').toString().trim()
+ // Use absolute path for security
+ const lddPath = '/usr/bin/ldd'
return readFileSync(lddPath, 'utf8').includes('musl')
} catch (e) {
+ console.warn('Failed to detect musl, assuming true for safety:', e.message)
return true
}
} else {
const { glibcVersionRuntime } = process.report.getReport().header
return !glibcVersionRuntime
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isMusl() { | |
| // For Node 10 | |
| if (!process.report || typeof process.report.getReport !== 'function') { | |
| try { | |
| const lddPath = require('child_process').execSync('which ldd').toString().trim() | |
| return readFileSync(lddPath, 'utf8').includes('musl') | |
| } catch (e) { | |
| return true | |
| } | |
| } else { | |
| const { glibcVersionRuntime } = process.report.getReport().header | |
| return !glibcVersionRuntime | |
| } | |
| } | |
| function isMusl() { | |
| // For Node 10 | |
| if (!process.report || typeof process.report.getReport !== 'function') { | |
| try { | |
| // Use absolute path for security | |
| const lddPath = '/usr/bin/ldd' | |
| return readFileSync(lddPath, 'utf8').includes('musl') | |
| } catch (e) { | |
| console.warn('Failed to detect musl, assuming true for safety:', e.message) | |
| return true | |
| } | |
| } else { | |
| const { glibcVersionRuntime } = process.report.getReport().header | |
| return !glibcVersionRuntime | |
| } | |
| } |
Summary by CodeRabbit
New Features
causeSigsegvandregisterfor handling segmentation faults.Configuration
Development
Documentation