Skip to content

src: do not call path.back() when it is empty #55072

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Sep 23, 2024

Fix a crash when running tests with the GN build of node:

../out/Debug/node test/parallel/test-cli-permission-multiple-allow.js

Crash trace:

$ lldb ../out/Debug/node
(lldb) target create "../out/Debug/node"
Current executable set to '/Users/zcbenz/codes/node_with_gn/out/Debug/node' (arm64).
(lldb) run --experimental-permission --allow-fs-write /Users/zcbenz/codes/node_with_gn/node/tmp --allow-fs-write /Users/zcbenz/codes/node_with_gn/node/other-path -e 'console.log(process.permission.has("fs.write"));'
Process 28010 launched: '/Users/zcbenz/codes/node_with_gn/out/Debug/node' (arm64)
../../third_party/gn/third_party/libc++/src/include/string:1364: assertion !empty() failed: string::back(): string is empty
Process 28010 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x000000018b88e0dc libsystem_kernel.dylib`__pthread_kill + 8
libsystem_kernel.dylib`:
->  0x18b88e0dc <+8>:  b.lo   0x18b88e0fc               ; <+40>
    0x18b88e0e0 <+12>: pacibsp 
    0x18b88e0e4 <+16>: stp    x29, x30, [sp, #-0x10]!
    0x18b88e0e8 <+20>: mov    x29, sp
Target 0: (node) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
  * frame #0: 0x000000018b88e0dc libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000018b8c5cc0 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x000000018b7d1a40 libsystem_c.dylib`abort + 180
    frame #3: 0x0000000103ddb5bc node`std::__Cr::__libcpp_verbose_abort(format="%s") at verbose_abort.cpp:74:3
    frame #4: 0x00000001004f0ef8 node`std::__Cr::basic_string<char, std::__Cr::char_traits<char>, std::__Cr::allocator<char>>::back(this="") const at string:1364:5
    frame #5: 0x000000010070bfd4 node`node::PathResolve(env=0x0000000134898a00, paths=size=1) at path.cc:269:55
    frame #6: 0x000000010070cd0c node`(anonymous namespace)::is_tree_granted(env=0x0000000134898a00, granted_tree=0x00006000006b44e8, param="") at fs_permission.cc:58:32
    frame #7: 0x000000010070cc2c node`node::permission::FSPermission::is_granted(this=0x00006000006b44d8, env=0x0000000134898a00, perm=kFileSystemWrite, param="") const at fs_permission.cc:168:15
    frame #8: 0x00000001000e0d10 node`node::permission::Permission::is_scope_granted(this=0x0000000134898f78, env=0x0000000134898a00, permission=kFileSystemWrite, res="") const at permission.h:83:33
    frame #9: 0x0000000100714878 node`node::permission::(anonymous namespace)::Has(v8::FunctionCallbackInfo<v8::Value> const&) [inlined] node::permission::Permission::is_granted(this=0x0000000134898f78, env=0x0000000134898a00, permission=kFileSystemWrite, res="") const at permission.h:56:12
    frame #10: 0x0000000100714844 node`node::permission::(anonymous namespace)::Has(args=0x000000016fdfa658) at permission.cc:58:55

Crash was introduced by #54224.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 23, 2024
@zcbenz zcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@avivkeller avivkeller added the path Issues and PRs related to the path subsystem. label Sep 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2024
@nodejs-github-bot

This comment was marked as outdated.

Copy link

codecov bot commented Sep 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (4f88179) to head (9edbef8).
Report is 123 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55072      +/-   ##
==========================================
- Coverage   88.23%   88.23%   -0.01%     
==========================================
  Files         652      652              
  Lines      183920   183920              
  Branches    35863    35868       +5     
==========================================
- Hits       162286   162280       -6     
+ Misses      14913    14912       -1     
- Partials     6721     6728       +7     
Files with missing lines Coverage Δ
src/path.cc 67.39% <100.00%> (ø)

... and 21 files with indirect coverage changes

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

@targos
Copy link
Member

targos commented Sep 23, 2024

Could we add a regression test?

@zcbenz
Copy link
Contributor Author

zcbenz commented Sep 23, 2024

Could we add a regression test?

The crash only happens when building with a newer version of libc++ that asserts on calling back() on an empty string, for the official build I don't see a way to test this.

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 25, 2024
@nodejs-github-bot nodejs-github-bot merged commit 773e7c6 into nodejs:main Sep 25, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 773e7c6

@targos targos added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Oct 4, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
PR-URL: nodejs#55072
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
PR-URL: nodejs#55072
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants