FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: node crashes due to multiline environment value by mertcanaltin · Pull Request #52266 · nodejs/node · GitHub

/ node Public

src: node crashes due to multiline environment value - #52266

Closed
mertcanaltin wants to merge 5 commits into
nodejs:mainfrom
mertcanaltin:dev-52248
ConversationCommits (5)ChecksFiles changed
Closed

src: node crashes due to multiline environment value#52266
mertcanaltin wants to merge 5 commits into
nodejs:mainfrom
mertcanaltin:dev-52248

Conversation

mertcanaltin commented Mar 29, 2024

Copy link
Copy Markdown
Member

Dotenv::ParseContent includes some improvements to avoid crash problems when processing a multiline value
#52248

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 Mar 29, 2024
Mert Can Altin added 2 commits March 29, 2024 22:43
kibertoad reviewed Mar 29, 2024
View reviewed changes
Comment thread test/parallel/test-dotenv.js Outdated
assert.strictEqual(process.env.MULTI_SINGLE_QUOTED, 'THIS\nIS\nA\nMULTILINE\nSTRING');
assert.strictEqual(process.env.MULTI_BACKTICKED, 'THIS\nIS\nA\n"MULTILINE\'S"\nSTRING');
assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '"');
assert.strictEqual(process.env.MULTI_NOT_VALID_QUOTE, '');

kibertoad Mar 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

does this cover all the added code?

IlyasShabi Mar 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The MULTI_NOT_VALID_QUOTE test makes sure that if there's a line starting with a double quote that doesn't close, we still count it as one single value.

MULTI_NOT_VALID_QUOTE="

IMO we should keep this test to make sure we handle these situations right

IlyasShabi Mar 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Dotenv interprets this as a single quote " too not an empty value

mertcanaltin Mar 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes this preserves the previous code

mertcanaltin Apr 6, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I must have done this wrong I will update it today

anonrig requested changes Mar 30, 2024
View reviewed changes

anonrig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a failing test?

mertcanaltin requested a review from anonrig March 30, 2024 07:26

mertcanaltin commented Mar 30, 2024

Copy link
Copy Markdown
Member Author

Can you add a failing test?

I wonder if it's normal to get fail afterwards 🤔

kibertoad mentioned this pull request Mar 30, 2024
Closed

marco-ippolito commented Mar 31, 2024

Copy link
Copy Markdown
Member

Can you add a failing test?

I wonder if it's normal to get fail afterwards 🤔

you can make sure it throws assert.throws

mertcanaltin commented Apr 6, 2024

Copy link
Copy Markdown
Member Author

I wanted to switch to the code in the main branch and test here, but I got such a test error

➜  node git:(dev-52248) ✗ ./node test/parallel/test-dotenv.js --NODE_SKIP_FLAG_CHECK
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ undefined
- 'basic'
    at Object.<anonymous> (/Users/mert/Desktop/openSource/node/test/parallel/test-dotenv.js:8:8)
    at Module._compile (node:internal/modules/cjs/loader:1421:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1499:10)
    at Module.load (node:internal/modules/cjs/loader:1232:32)
    at Module._load (node:internal/modules/cjs/loader:1048:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:187:14)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: 'basic',
  operator: 'strictEqual'
}

Node.js v22.0.0-pre

mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2024

nodejs-github-bot commented Apr 6, 2024

Copy link
Copy Markdown
Collaborator

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

IlyasShabi mentioned this pull request Apr 7, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

IlyasShabi IlyasShabi left review comments
anonrig Awaiting requested review from anonrig
+1 more reviewer
kibertoad kibertoad left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. 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.

6 participants

Back | FazBrowse Home | New Git URL