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

src: safely remove the last line from dotenv #55982

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

islandryu
Copy link
Contributor

Refs: #55925

When _GLIBCXX_ASSERTIONS is enabled, passing npos to remove_prefix triggers an assertion error. Modified to avoid calling remove_prefix when npos is encountered.

#0  0x0000ffffa799e020 in __pthread_kill_implementation () from /lib64/libc.so.6
#1  0x0000ffffa794b140 [PAC] in raise () from /lib64/libc.so.6
#2  0x0000ffffa79359c8 [PAC] in abort () from /lib64/libc.so.6
#3  0x0000ffffa7cc20c8 [PAC] in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
#4  0x000000000195c634 [PAC] in std::basic_string_view<char, std::char_traits<char> >::remove_prefix (this=0xffffd0a72938, __n=18446744073709551615) at /usr/include/c++/14/string_view:297
#5  0x000000000195b98c [PAC] in node::Dotenv::ParseContent (this=0x8c3adf8 <node::per_process::dotenv_file>, input="API_KEY=\"inaaa\"") at ../../src/node_dotenv.cc:185
#6  0x000000000195be60 [PAC] in node::Dotenv::ParsePath (this=0x8c3adf8 <node::per_process::dotenv_file>, path=".env") at ../../src/node_dotenv.cc:276
#7  0x00000000018dceb4 [PAC] in node::InitializeNodeWithArgsInternal (argv=0xe11eda0, exec_argv=0xe11edb8, errors=0xe11edd0, flags=node::ProcessInitializationFlags::kNoFlags) at ../../src/node.cc:860
#8  0x00000000018dd650 [PAC] in node::InitializeOncePerProcessInternal (args=std::vector of length 2, capacity 2 = {...}, flags=node::ProcessInitializationFlags::kNoFlags) at ../../src/node.cc:1003
#9  0x00000000018de97c [PAC] in node::StartInternal (argc=2, argv=0xe11ece0) at ../../src/node.cc:1432
#10 0x00000000018ded04 [PAC] in node::Start (argc=2, argv=0xffffd0a753f8) at ../../src/node.cc:1491
#11 0x0000000003935f54 [PAC] in main (argc=2, argv=0xffffd0a753f8) at ../../src/[node_main.cc:97](http://node_main.cc:97/)

In the case of Windows, a debug build is likely to emit a warning when npos is passed to remove_prefix.

@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 Nov 24, 2024
Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (1919560) to head (c64de0c).
Report is 27 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55982      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.01%     
==========================================
  Files         653      653              
  Lines      187872   188097     +225     
  Branches    35888    35942      +54     
==========================================
+ Hits       165328   165522     +194     
- Misses      15720    15738      +18     
- Partials     6824     6837      +13     
Files with missing lines Coverage Δ
src/node_dotenv.cc 92.77% <100.00%> (+0.24%) ⬆️

... and 30 files with indirect coverage changes

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

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added lts-watch-v22.x PRs that may need to be released in v22.x dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 25, 2024
@anonrig
Copy link
Member

anonrig commented Nov 25, 2024

It's weird that some compilers have internal assertions that cause such harm and meanwhile some just don't care...

@richardlau
Copy link
Member

It's weird that some compilers have internal assertions that cause such harm and meanwhile some just don't care...

The assertions aren't causing harm -- in this case it highlighted a real bug. _GLIBCXX_ASSERTIONS isn't enabled by default either -- downstream repackagers (e.g. Fedora, Arch) are turning it on in their rebuilds of Node.js.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 26, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3bb1d28 into nodejs:main Nov 26, 2024
84 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3bb1d28

aduh95 pushed a commit that referenced this pull request Nov 26, 2024
Refs: #55925
PR-URL: #55982
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.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. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants