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

cgi-bin-luci: set explicit PATH when run as CGI #1048

Closed
wants to merge 1 commit into from

Conversation

gstrauss
Copy link

@gstrauss gstrauss commented Mar 2, 2017

cgi-bin-luci: set explicit PATH when run as CGI

github: fixes #1922

Signed-off-by: Glenn Strauss gstrauss@gluelogic.com

@hnyman
Copy link
Contributor

hnyman commented Mar 2, 2017

Did you intentionally select a different PATH contents /sbin:/usr/sbin:/bin:/usr/bin than the standard PATH in Openwrt & LEDE masters PATH=/usr/sbin:/usr/bin:/sbin:/bin ?

Reference to:

That may lead to unexpected results, as PATH is used to set precedence among co-existing variants of some tools (e.g. full wget before busybox wget etc.).

github: fixes openwrt#1922

Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
@gstrauss
Copy link
Author

gstrauss commented Mar 2, 2017

No, the difference in PATH is not intentional. I just fixed it to match and force-pushed an update to the pull request. Thanks for catching that.

@hnyman
Copy link
Contributor

hnyman commented Mar 2, 2017

@jow-
You know better than me if this is a good solution.

@gstrauss
Copy link
Author

@jow- ping. Would you please take a look at this for hnyman and provide feedback? Thank you.

@gstrauss
Copy link
Author

@jow- I believe you included patches in openwrt/packages#1922 which addressed the underlying reason for this request, but still think it is good practice to explicitly set PATH for a script expected to run with root privileges.

@gstrauss
Copy link
Author

@jow- I believe you included patches in openwrt/packages#1922 which addressed the underlying reason for this request, but still think it is good practice to explicitly set PATH for a script expected to run with root privileges.

@@ -1,4 +1,10 @@
#!/usr/bin/lua
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of my experience, but it looks a bit strange to me that the shebang is changed from lua to sh, when the majority of the code below is still lua.

Copy link
Author

Choose a reason for hiding this comment

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

@hnyman: The shell script sets the PATH and then invokes lua. It is written as valid lua code. To my knowledge, it is not possible to set PATH in lua. This pull request is a portable way to do so for many/all versions of lua.

@jow-
Copy link
Contributor

jow- commented May 3, 2017

I do not like the path override here, especially as it does not adhere to the value of the CONFIG_TARGET_INIT_PATH option in LEDE / OpenWrt.

Since the underlying issue is closed and since CGI does not guarantee a sane PATH value, I'll close this without merging.

@jow- jow- closed this May 3, 2017
mmatuska pushed a commit to mmatuska/opkg that referenced this pull request Aug 3, 2018
avoid strange behavior with execvp() when PATH is not set in environment
(in which case confstr(_CS_PATH) should return something reasonable)

reproducable running openwrt 15.05 and 15.05.1 and attempting to install
a software package (e.g. libuuid) via LuCI (prior to openwrt/luci#1048).
(openwrt/luci#1048) libuuid.postinst fails with
status 255 on 15.05 and opkg segfaults in 15.05.1.  This probably merits
further exploration.

Originally reported in openwrt/packages#1922

Signed-off-by: Glenn Strauss <gstrauss@gluelogic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS and Route gets deleted after save and apply is pressed in luci-app-ocserv
3 participants