Skip to content

src: fix accessing empty string - #57014

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
zcbenz:empty-string-access
Feb 16, 2025
Merged

src: fix accessing empty string#57014
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
zcbenz:empty-string-access

Conversation

@zcbenz

@zcbenzzcbenz commented Feb 12, 2025

Copy link
Copy Markdown
Contributor

Fix an assertion caused by accessing empty string:

(lldb) run -e "process.loadEnvFile('./test/fixtures/dotenv/eof-without-value.env')"
../../third_party/gn/third_party/libc++/src/include/string_view:411: assertion !empty() failed: string_view::front(): string is empty
(lldb) bt
* thread #1, name = 'MainThread', queue = 'com.apple.main-thread', stop reason = signal SIGABRT
* frame #0: 0x00007ff81b4dfc52 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007ff81b519f85 libsystem_pthread.dylib`pthread_kill + 262
frame #2: 0x00007ff81b43ab19 libsystem_c.dylib`abort + 126
frame #3: 0x0000000101b96cb2 node`std::__Cr::__libcpp_verbose_abort(format=<unavailable>) at verbose_abort.cpp:74:3 [opt]
frame #4: 0x00000001000fe088 node`node::Dotenv::ParseContent(std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>) [inlined] std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>::front(this=<unavailable>) const at string_view:411:12 [opt]
frame #5: 0x00000001000fe073 node`node::Dotenv::ParseContent(this=0x00007ff7bfefd670, input=<unavailable>) at node_dotenv.cc:159:17 [opt]
frame #6: 0x00000001000fe4dd node`node::Dotenv::ParsePath(this=0x00007ff7bfefd670, path=<unavailable>) at node_dotenv.cc:296:3 [opt]
frame #7: 0x000000010018f3e7 node`node::LoadEnvFile(args=0x00007ff7bfefdac8) at node_process_methods.cc:488:18 [opt]

@nodejs-github-botnodejs-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 Feb 12, 2025
@codecov

codecovBot commented Feb 12, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.08%. Comparing base (579fc67) to head (f5ce221).
Report is 2 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57014 +/- ##
==========================================
- Coverage 89.08% 89.08% -0.01% 
==========================================
Files 665 665 Lines 193248 193248 Branches 37229 37229 ==========================================
- Hits 172157 172151 -6 - Misses 13806 13821 +15 + Partials 7285 7276 -9
Files with missing linesCoverage Δ
src/node_dotenv.cc89.61% <100.00%> (-1.64%)⬇️

... and 25 files with indirect coverage changes

@joyeecheung

joyeecheung commented Feb 12, 2025

Copy link
Copy Markdown
Member

Can you add a test for this?

@zcbenz

zcbenz commented Feb 12, 2025

Copy link
Copy Markdown
ContributorAuthor

Can you add a test for this?

The assertion happened when running tests, when built with newer version of libc++ in GN build. So it is already being tested.

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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@zcbenzzcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Feb 15, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65218/

@jasnell

jasnell commented Feb 15, 2025

Copy link
Copy Markdown
Member

The windows CI failures appear to be persistent and consistent. Can you please try rebasing this on main before we run again in case there's something not matching up. @nodejs/platform-windows any ideas?

@nodejs-github-bot

nodejs-github-bot commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65239/

@nodejs-github-bot

nodejs-github-bot commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65242/

@nodejs-github-bot

nodejs-github-bot commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65244/

@zcbenzzcbenz added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 16, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 16, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 1d8593e into nodejs:mainFeb 16, 2025
@nodejs-github-bot

nodejs-github-bot commented Feb 16, 2025

Copy link
Copy Markdown
Collaborator

Landed in 1d8593e

@zcbenz
zcbenz deleted the empty-string-access branch February 16, 2025 06:48
targos pushed a commit that referenced this pull request Feb 17, 2025
@zcbenz@targos
src: fix accessing empty string
286bb84
PR-URL: #57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@zcbenz@acidiney
src: fix accessing empty string
484ee07
PR-URL: nodejs#57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actionsgithub-actionsBot mentioned this pull request Feb 25, 2025
Merged
aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@zcbenz@aduh95
src: fix accessing empty string
dea827a
PR-URL: #57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@zcbenz@aduh95
src: fix accessing empty string
38797a8
PR-URL: #57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95aduh95 mentioned this pull request Apr 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@zcbenz@RafaelGSS
src: fix accessing empty string
af61e98
PR-URL: #57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@zcbenz@RafaelGSS
src: fix accessing empty string
e05e2cf
PR-URL: #57014
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@lpincalpincalpinca approved these changes
@anonriganonriganonrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
@UlisesGasconUlisesGasconUlisesGascon approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@zcbenz@joyeecheung@nodejs-github-bot@jasnell@addaleax@lpinca@anonrig@UlisesGascon