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: clean up SafeGetenv() #13220

Merged
merged 1 commit into from May 29, 2017

Conversation

@cjihrig
Copy link
Contributor

commented May 25, 2017

This commit:

  • Reduces duplicated code.
  • Only checks linux_at_secure on Linux (it's false otherwise).
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@refack
refack approved these changes May 25, 2017
@danbev
danbev approved these changes May 26, 2017
src/node.cc Outdated
return false;
}

#if defined(__linux__)

This comment has been minimized.

Copy link
@danbev

danbev May 26, 2017

Member

I might have misunderstood the code review but it was intentional to not have the macro guard (__linux__) here. Perhaps the variable should also be guarded in node_main.cc and node.cc?

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 26, 2017

Author Contributor

Hm, we could skip the #ifdef __linux__ ugliness mentioned there, and just add linux_at_secure into the existing if (getuid() != geteuid() || getgid() != getegid()) check. Whichever you prefer.

This comment has been minimized.

Copy link
@danbev

danbev May 27, 2017

Member

I'd opt for not having the #ifdef if that is acceptable.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 27, 2017

Member

Yes, the idea was to have as little code behind ifdefs as possible.

@cjihrig cjihrig force-pushed the cjihrig:safegetenv branch May 29, 2017

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

@mhdawson
Copy link
Member

left a comment

LGTM

src: reduce duplicate code in SafeGetenv()
PR-URL: #13220
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@cjihrig cjihrig force-pushed the cjihrig:safegetenv branch to 88fe7e8 May 29, 2017

@cjihrig cjihrig merged commit 88fe7e8 into nodejs:master May 29, 2017

@cjihrig cjihrig deleted the cjihrig:safegetenv branch May 29, 2017

jasnell added a commit that referenced this pull request May 29, 2017
src: reduce duplicate code in SafeGetenv()
PR-URL: #13220
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Should this land on v6.x?

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Aug 14, 2017

ping @cjihrig

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2017

It can. It's not critical though.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

this is not landing cleanly. Putting "don't land" for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.