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.

Sign up for GitHub

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

Jump to bottom

os,contextify: fix segfaults and CHECK failure #12371

Closed
wants to merge 3 commits into from
Closed

os,contextify: fix segfaults and CHECK failure #12371

wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Apr 12, 2017 •
edited
Loading

Fixes multiple possible segmentation faults in node_contextify.cc and an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

os, vm, contextify

@tniessen
os,contextify: fix segfaults and CHECK failure
225fd64 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: nodejs#12369
Fixes: nodejs#12370
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem. vm Issues and PRs related to the vm subsystem. labels Apr 12, 2017
@Trott
Copy link
Member

Trott commented Apr 12, 2017

Request: Add a test?

addaleax
addaleax approved these changes Apr 12, 2017
View reviewed changes
src/node_contextify.cc Outdated
Local<Value> value = options.As<Object>()->Get(key);
return value->IsTrue();
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(), key);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe maybe_value? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

node_contextify.cc is pretty inconsistent about this, even within functions 😞 By the way, why don't we have linting for variable names?

If you prefer, I will change it :)

Copy link
Member

Choose a reason for hiding this comment

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

By the way, why don't we have linting for variable names?

I guess it’s mostly because there are only a handful of people contributing to the C++ sources on a regular basis, so adding linter rules comes with a disproportionate overhead when compared to e.g. the JS sources…

If you prefer, I will change it :)

I don’t care much personally, but I’d bet @bnoordhuis is going to request it. 😄

src/node_contextify.cc Outdated
MaybeLocal<Value> maybeValue =
options.As<Object>()->Get(env->context(),
env->produce_cached_data_string());
if (maybeValue.IsEmpty())
Copy link
Member

Choose a reason for hiding this comment

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

(ditto everywhere, you may want to search+replace for maybeValue)

@addaleax addaleax mentioned this pull request Apr 12, 2017
Closed
@tniessen
Copy link
Member Author

@Trott Thank you, done.

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/9115/

@tniessen
Copy link
Member Author

Thanks Anna, CI is green 😃

@addaleax
Copy link
Member

Thanks, landed in 88aab45.

I noticed while landing that the commit message had the subsystem contextify, which belongs to the vm module and therefore has gotten that label (at least the last few times it was touched), so I changed that. :)

@addaleax addaleax closed this Apr 14, 2017
addaleax pushed a commit that referenced this pull request Apr 14, 2017
@tniessen @addaleax
os,vm: fix segfaults and CHECK failure
88aab45 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
@tniessen @evanlucas
os,vm: fix segfaults and CHECK failure
cf6c29c 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@tniessen @evanlucas
os,vm: fix segfaults and CHECK failure
354df94 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@tniessen @evanlucas
os,vm: fix segfaults and CHECK failure
6998e80 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

Should this land on v6.x??? It isn't landing cleanly and will require a manual backport if so

@tniessen
Copy link
Member Author

That's up to you folks, I think bug fixes should be welcome in LTS versions, but this is not an urgent fix. I don't know how much more work it would take, I would be happy to help though (if I can). If you decide that inclusion in v6.x is not worth the additional efforts, that's fine by me.

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

If you're willing to backport to v6.x-staging please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

tniessen added a commit to tniessen/node that referenced this pull request Jun 22, 2017
@tniessen
os,vm: fix segfaults and CHECK failure
408d69b 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: nodejs#12369
Fixes: nodejs#12370
PR-URL: nodejs#12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@tniessen tniessen mentioned this pull request Jun 22, 2017
Closed
4 tasks
@tniessen
Copy link
Member Author

@gibfahn #13871

MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
@tniessen @MylesBorins
os,vm: fix segfaults and CHECK failure
c29063d 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
Backport-PR-URL: #13871
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@tniessen @MylesBorins
os,vm: fix segfaults and CHECK failure
47e7020 
Fixes multiple possible segmentation faults in node_contextify.cc and
an assertion failure in node_os.cc

Fixes: #12369
Fixes: #12370
Backport-PR-URL: #13871
PR-URL: #12371
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
@tniessen tniessen mentioned this pull request Jan 2, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. os Issues and PRs related to the os subsystem. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

os.userInfo can cause segmentation fault Multiple segmentation faults in contextify when parsing options
6 participants
@tniessen @Trott @addaleax @MylesBorins @gibfahn @nodejs-github-bot