[GitHub] [maven] jmtd commented on pull request #556: [MNG-7270] Switch to shell alternative to "which"

[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