You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/09/29 11:40:09 UTC
[GitHub] [maven] jmtd opened a new pull request #556: [MNG-7270] Switch to shell alternative to "which"
jmtd opened a new pull request #556:
URL: https://github.com/apache/maven/pull/556
In some circumstances the init script called `which`, which may or may
not be available on the host system. Instead, use `command -v`, which is
nearly equivalent. One area it differs is if the command being queried
is defined as a shell alias. To avoid that, call `unalias -a` to
undefine any aliases in the subshell. Also call `unset -f command` to
avoid the situation where "command" has been re-defined as a shell
function.
See here for more information on this approach:
<https://pubs.opengroup.org/onlinepubs/009695399/utilities/command.html>
Tested with bash, sh (bash invoked as sh), posh, dash, zsh and mksh.
https://issues.apache.org/jira/browse/MNG-7270
`mvn clean verify` passed.
I've signed the CLA.
----
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] jmtd commented on pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
jmtd commented on pull request #556:
URL: https://github.com/apache/maven/pull/556#issuecomment-931219516
> I tried your script on the POSIX shell on HP-UX, the result is quite surpring:
Thanks! I no longer have (easy) access to the less common UNIXes. (My go-to- awkward shell for testing things would have been Solaris sh <10).
> What would be your opinon on that?
It's good news, it means on that shell the environment could not have re-defined "command", and the mitigation `\\unset -f command` would not be necessary, but hasn't caused problems either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit closed pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #556:
URL: https://github.com/apache/maven/pull/556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] asfgit closed pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #556:
URL: https://github.com/apache/maven/pull/556
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #556:
URL: https://github.com/apache/maven/pull/556#discussion_r718427758
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all aliases? Do you assume `java(1)` to be an alias?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #556:
URL: https://github.com/apache/maven/pull/556#discussion_r718427758
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all aliases? Do you assume `java(1)` to be an alias?
* `unset` is to use the shell builtin only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #556:
URL: https://github.com/apache/maven/pull/556#discussion_r718427758
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume as `unalias` could be an alias as well?
* Why are you unset all alias? Do you assume `java(1)` to be an alias?
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all alias? Do you assume `java(1)` to be an alias?
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all aliases? Do you assume `java(1)` to be an alias?
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all aliases? Do you assume `java(1)` to be an alias?
* `unset` is to use the shell builtin only?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #556:
URL: https://github.com/apache/maven/pull/556#discussion_r718427758
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume that `unalias` could be an alias as well?
* Why are you unset all alias? Do you assume `java(1)` to be an alias?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a change in pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on a change in pull request #556:
URL: https://github.com/apache/maven/pull/556#discussion_r718427758
##########
File path: apache-maven/src/assembly/shared/init
##########
@@ -20,7 +20,7 @@ if [ -n "$JAVA_HOME" ] ; then
fi
fi
else
- JAVACMD="`which java`"
+ JAVACMD="`\\unalias -a; unset -f command; command -v java`"
Review comment:
* Do you assume as `unalias` could be an alias as well?
* Why are you unset all alias? Do you assume `java(1)` to be an alias?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] jmtd commented on pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
jmtd commented on pull request #556:
URL: https://github.com/apache/maven/pull/556#issuecomment-931290865
I'm not sure whether this level of complication is warranted in the patch itself (and I'd love your input on that), but, via [1](https://unix.stackexchange.com/a/188365), the following resolves the "someone has redefined unset" problem in non-POSIX bash (but not zsh):
echo "`POSIXLY_CORRECT=1 \\unset -f command; \\command -v java`"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] jmtd commented on pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
jmtd commented on pull request #556:
URL: https://github.com/apache/maven/pull/556#issuecomment-931189840
Hi @michael-o thanks for your comments. The approach I used was lifted straight from the opengroup.org URI in the PR body/commit. However, I realised last night that I'd made a mistake in my local testing, so I have revised and force pushed.
To answer your questions
> Do you assume that unalias could be an alias as well?
Yes, and the (escaped) backslash prefix mitigates that,
> Why are you unset all aliases? Do you assume java(1) to be an alias?
Simply because it's easier than specifying the ones to unset. The sub-shell context inside the backticks doesn't leak out to the wider init script, so that `unset` doesn't take effect later on in the script.
> unset is to use the shell builtin only?
Yes. At least to attempt to!
The mistake in my local testing related to `unalias`. I found that it's just not reliable in all situations, and using the backslash prefix is preferable. Here's a script demonstrating an adversarial environment and the results for a bunch of shells:
```
$ cat adversarial.sh
command() { echo INTERCEPTED 2; }
alias command="echo INTERCEPTED 1"
echo "`\\unset -f command; \\command -v java`"
$ for shell in bash 'bash --posix' posh dash zsh mksh 'busybox sh'; do echo -n "$shell: "; $shell adversarial.sh; done
bash: /usr/bin/java
bash --posix: /usr/bin/java
posh: adversarial.sh:2: alias: not found
/usr/bin/java
dash: /usr/bin/java
zsh: /usr/bin/java
mksh: /usr/bin/java
busybox sh: /usr/bin/java
```
The situation that I am aware of that isn't covered is if the adversarial environment has shadowed `unset`. This is a syntax error in most of the shells above (including bash in --posix mode) but is possible in bash and zsh. The mitigation would be to use `builtin`. However, that's a shell extension and not in POSIX. And, you can shadow `builtin` too! I haven't figured out a defence against that. Having said that, my reading of [the relevant document](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01) is that shell built-ins should have precedence over functions when resolving names anyway.
The mitigation is this init script ends up prefixed with `#!/bin/sh`, which if provided by bash, triggers POSIX mode. I've tested this patch on a Debian system where `/bin/sh` is provided by `dash`, and a RHEL system where it's provided by `bash` (same on macos).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o edited a comment on pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o edited a comment on pull request #556:
URL: https://github.com/apache/maven/pull/556#issuecomment-931216010
I tried your script on the POSIX shell on HP-UX, the result is quite surpring:
```
$ LC_ALL=C.utf8 ./script
./script[3]: restricted: command is a shell builtin
/opt/java8/bin/java
```
What would be your opinon on that?
The offending line is: `command() { echo INTERCEPTED 2; }`
Counter example the POSIX shell on FreeBSD 12-STABLE:
```
$ ./script
/usr/local/bin/java
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #556: [MNG-7270] Switch to shell alternative to "which"
Posted by GitBox <gi...@apache.org>.
michael-o commented on pull request #556:
URL: https://github.com/apache/maven/pull/556#issuecomment-931216010
I tried your script on the POSIX shell on HP-UX, the result is quite surpring:
```
$ LC_ALL=C.utf8 ./script
./script[3]: restricted: command is a shell builtin
/opt/java8/bin/java
```
What would be your opinon on that?
Counter example the POSIX shell on FreeBSD 12-STABLE:
```
$ ./script
/usr/local/bin/java
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org