Ticket #3689 (closed enhancement: duplicate)
mc doesn't recognize shell (e.g., /bin/sh) as bash when it is a link to bash binary
| Reported by: | alllexx88 | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | |
| Component: | mc-core | Version: | master |
| Keywords: | shell bash | Cc: | egmont |
| Blocked By: | Blocking: | ||
| Branch state: | no branch | Votes for changeset: |
Description
Currently mc identifies shell as bash only if this statement is true (in lib/shell.c):
(strstr (mc_shell->path, "/bash") != NULL getenv ("BASH") != NULL)
However, this won't work if shell, e.g., is /bin/sh pointing to bash. Let's see why:
- (strstr (mc_shell->path, "/bash") != NULL) is obviously false, since shell path doesn't end with "/bash"
- (getenv ("BASH") != NULL) is false too, unless BASH env variable was somewhere intentionally set, since BASH is actually internal bash variable -- not environmental variable, and so by default getenv ("BASH") is NULL
Here's a log that illustrates the second point:
root@DiskStation:/opt# cat test.c
#include <stdio.h> /* printf */
#include <stdlib.h> /* getenv */
int main ()
{
char* bash;
bash = getenv ("BASH");
if (bash!=NULL)
printf ("$BASH=%s\n",bash);
return 0;
}
root@DiskStation:/opt# gcc test.c -o test
root@DiskStation:/opt# echo $BASH
/bin/ash
root@DiskStation:/opt# env
SHELL=/bin/ash
TERM=xterm
SSH_CLIENT=192.168.133.169 63110 22
SSH_TTY=/dev/pts/5
LC_ALL=en_US.utf8
USER=root
PAGER=more
PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/syno/sbin:/usr/syno/bin:/usr/local/sbin:/usr/local/bin:/opt/bin:/opt/sbin
MAIL=/var/mail/root
PWD=/opt
EDITOR=/opt/bin/nano
LANG=en_US.utf8
PS1=\[\033[01;32m\]\u@\h\[\033[00m\]:\[\033[01;34m\]\w\[\033[00m\]\$
HOME=/root
SHLVL=2
TERMINFO=/usr/share/terminfo
LOGNAME=root
SSH_CONNECTION=192.168.133.169 63110 192.168.133.15 22
PGDATA=/var/services/pgsql
_=/bin/env
root@DiskStation:/opt# ./test
root@DiskStation:/opt# export BASH=$BASH
root@DiskStation:/opt# ./test
$BASH=/bin/ash
root@DiskStation:/opt# /bin/ash --version
GNU bash, version 4.3.39(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
This is a log from Synology DSM 6.0. It uses /bin/ash as login shell for root user, and /bin/sh -- for admin user. Both /bin/ash and /bin/sh are links to bash. Starting mc as root leads to identifying shell as busybox ash, and as admin -- leads to 'common.c: unimplemented subshell type 1' error and no subshell as the result.
The suggested fix (attaching patch next) is to test whether mc_shell->real_path ends with "/bash". Tested to work fine on Synology DSM 6.0.
Attachments
Change History
comment:2 Changed 8 years ago by alllexx88
| exit 0", (char *) NULL) in a child fork and has parent evaluate its exit code with waitpid(). I have tested this on different corner cases, like execl() failing, or program passed as SHELL not accepting the syntax. In theory, if SHELL is set as something intentionally malicious, execl() here can do bad things, but we will still do them, if shell is recognized as supported (and that's easy to achieve with a symlink), when we try to initialize it afterwards. |
Will be thankful for your feedback
comment:3 follow-up: ↓ 5 Changed 8 years ago by egmont
bash behaves differently (in some sh-compatible way) if invoked as sh. Are you sure it's okay to recognize it as bash then? Wouldn't it result in mc expecting certain things that it deliberate refuses to do?
comment:5 in reply to: ↑ 3 Changed 8 years ago by alllexx88
Replying to egmont:
bash behaves differently (in some sh-compatible way) if invoked as sh. Are you sure it's okay to recognize it as bash then? Wouldn't it result in mc expecting certain things that it deliberate refuses to do?
That's a good point. To answer this, I used grep:
$ grep SHELL_BASH `find -type f -name '*.[ch]'` ./src/subshell/common.c: case SHELL_BASH: ./src/subshell/common.c: case SHELL_BASH: ./src/subshell/common.c: case SHELL_BASH: ./lib/shell.h: SHELL_BASH, ./lib/shell.c: mc_shell->type = SHELL_BASH;
lib/shell.h defines shell types enum, lib/shell.c tries to guess the type. So, we should only care about src/subshell/common.c here. After looking at it, it's clear that the only two things unique to SHELL_BASH are:
case SHELL_BASH:
g_snprintf (precmd, buff_size,
" PROMPT_COMMAND=${PROMPT_COMMAND:+$PROMPT_COMMAND\n}'pwd>&%d;kill -STOP $$'\n"
"PS1='\\u@\\h:\\w\\$ '\n", subshell_pipe[WRITE]);
break;
in function init_subshell_precmd
and
case SHELL_BASH:
/* Do we have a custom init file ~/.local/share/mc/bashrc? */
init_file = mc_config_get_full_path ("bashrc");
/* Otherwise use ~/.bashrc */
if (!exist_file (init_file))
{
g_free (init_file);
init_file = g_strdup (".bashrc");
}
/* Make MC's special commands not show up in bash's history and also suppress
* consecutive identical commands*/
putenv ((char *) "HISTCONTROL=ignoreboth");
/* Allow alternative readline settings for MC */
{
char *input_file;
input_file = mc_config_get_full_path ("inputrc");
if (exist_file (input_file))
{
putenv_str = g_strconcat ("INPUTRC=", input_file, (char *) NULL);
putenv (putenv_str);
}
g_free (input_file);
}
break;
...
case SHELL_BASH:
execl (mc_global.shell->path, mc_global.shell->name, "-rcfile", init_file, (char *) NULL);
break;
in init_subshell_child (BTW, init_file = g_strdup (".bashrc") should be init_file = g_strdup_printf ("%s/.bashrc", g_getenv ("HOME")), I think: see https://www.midnight-commander.org/ticket/3684#comment:10)
So, it only affects subshell initialization, which apparently succeeds: I tested it with SHELL=/bin/sh, where sh is linked to bash. Beyond that, no assumptions are made. I should note that bash invoked as sh doesn't load ~/.bashrc by default (nor ~/.profile, /etc/profile or anything else -- at least, according to strace), but we would rather have these defaults loaded if present, wouldn't we?
Thank you for your reply
comment:7 in reply to: ↑ 6 Changed 8 years ago by alllexx88
Replying to egmont:
Sounds good, thanks for checking this! :)
No problem, it was a very valid point :)
Another thing I came to think of: bash must have BASH *internal* variable set, however, the test function will report it set if there is BASH environmental variable set too, even when there's no such internal variable, thus falsely assuming it bash. Maybe we should do unsetenv() in the child fork to make sure this doesn't happen? What do you think? I have to go to sleep now, will try and test it when I have time tomorrow.
Thanks
comment:8 Changed 8 years ago by alllexx88
Here's V3 of the patch. It's the same as V2, except that we make sure we test for internal variable by unsetting environmental variable of the same name in the child fork first. I think, this is the best test for bash we can use: shell is bash if and only if it has BASH internal variable. Actually, this test function can be used to identify other shells too, like testing for ZSH_NAME internal variable to see if we have zsh. This approach is more robust than testing for binary names, and should be applicable to most of the supported shells.
What do you think about this?
Thanks
comment:9 Changed 8 years ago by alllexx88
@egmont There's a small bug in the V2 and V3 patches. However, I went further than just fixing them. I changed shell detection mechanism to rely rather on specific variables that are unique to respective shell, rather than testing paths. It's possible to do this safe enough for bash, zsh, fish, tcsh and ordinary csh. With bsd-csh it's a bit more complicated: I test for shell variable (lower register -- that's important), and whether it errors on undefined variable without set -u -- and if both conditions are true, this is bsd-csh. I don't think we can have a better test for it. Only dash and busybox ash have no special variables, and so are still detected using paths. I've been testing this patch recently, and so far it seems to be robust. I'll be testing it for a day or two more, and then open a new ticket, since this change goes beyond this one. Should I close this one for the time being?
Thanks,
Alex
comment:11 Changed 8 years ago by alllexx88
I haven't figured out how to close this ticket, and it looks like I can't (while it's new?). Please discard this ticket in favour of this one: https://www.midnight-commander.org/ticket/3692
comment:12 Changed 8 years ago by andrew_b
- Status changed from new to closed
- Resolution set to duplicate
- Milestone Future Releases deleted
Closed as duplicate of #3658.
