Thank you for working on this though,

The chsh command here is in need of new fixes/features/functionality anyway,
though this PR is a start, do you have any idea how to maybe fix this other problem also?

• "chsh -s path/to/shell" argument handling differs from regular chsh #140

It would be useful and helpful to users coming from GNU/Linux, I think, if the chsh command could accept an absolute path to a shell, but also preserve backwards compatibility with the current ability to set the path relative to $PREFIX/bin, by checking the given path to determine whether it exists as a path relative to $PREFIX/bin first, and then if an executable file does not exist there, then check as an alternative whether it exists as an executable file at an absolute path.

There is, however, also the factor to consider that the original creator of login, Fornwall, might have used the -G argument to test for a security reason of some kind, to try to make sure that the shell executed always has the same Group Ownership as the Termux user. 539f128

This detail could be relevant for security, but from a usability perspective, it makes it inconvenient and tedious to set programs outside of the Termux folder as the default shell if desired, like for example the global mksh, /system/bin/sh.

I would be interested in a discussion about the potential benefits or drawbacks of relaxing the strictness of the permissions check in login on the shell set in ~/.termux/shell.

Thanks for your comments.
I'll need to have a think about absolute paths in chsh, but it sounds like a good idea.

Very good! I like this!

I just have a couple of other small suggestions, and then I don't see anything else I would change here.

Other people might have other comments about this though. I am not sure what fornwall's or agnostic-apollo's opinions about this would be since they designed the original behavior, and they might have chosen it for a reason.

@TomJo2000 as far as I can see, every single thing you mentioned is something that is already there in the current master branch of termux-tools before any of the changes in this PR (so everything you mentioned is copied and pasted from the master branch).

Does this mean you would like everything you mentioned to be changed globally throughout the repository, all at once?

@TomJo2000 as far as I can see, every single thing you mentioned is something that is already there in the current master branch of termux-tools before any of the changes in this PR (so everything you mentioned is copied and pasted from the master branch).

Does this mean you would like everything you mentioned to be changed globally throughout the repository, all at once?

Is this PR not up to date with the master/HEAD commit of this repository?

Is this PR not up to date with the master/HEAD commit of this repository?

It is (otherwise GitHub would say there are conflicts)

what I mean to say is that every change you suggested (except for the double-exporting of SHELL) could also be said of the master branch, so should this PR also apply the suggested changes to every single place they could be applied throughout the termux-tools repository?

what I mean to say is that every change you suggested (except for the double-exporting of SHELL) could also be said of the master branch, so should this PR also apply the suggested changes to every single place they could be applied throughout the termux-tools repository?

We should clean up quoting across all scripts.
I thought I was only commenting on newly introduced code though.

I thought I was only commenting on newly introduced code though.

For each case of green (added) "unquoted variables", "backtick command substitution", and "-G argument to test" you mentioned, there is a corresponding red (removed) case of the same thing, indicating that it was already present in the master branch and was copied and pasted from there directly into the "new code" in this PR.

In my opinion, it would be more organized to make all those changes in a separate PR to keep them together, and keep this PR focused on solving these two issues:

• "chsh -s path/to/shell" argument handling differs from regular chsh #140

• bump(main/busybox): 1.37.0 termux-packages#24986

That way, the changes are individually readable and reviewable more easily, and whichever of the two PRs is merged last, is the PR that would end up having to be rebased over the changes added into master by the other PR.

I thought I was only commenting on newly introduced code though.

For each case of green (added) "unquoted variables", "backtick command substitution", and "-G argument to test" you mentioned, there is a corresponding red (removed) case of the same thing, indicating that it was already present in the master branch and was copied and pasted from there directly into the "new code" in this PR.

Eh, didn't notice that I guess.
But yes, we should absolutely not be using test -G in a #!/bin/sh script.

In my opinion, it would be more organized to make all those changes in a separate PR to keep them together, and keep this PR focused on solving these two issues:

• "chsh -s path/to/shell" argument handling differs from regular chsh #140
• fix(main/busybox): add ash symlink termux-packages#24986

That makes sense to me.
I did not intend to expand the scope of this PR.

That way, the changes are individually readable and reviewable more easily, and whichever of the two PRs is merged last, is the PR that would end up having to be rebased over the changes added into master by the other PR.

Yep that makes more sense.
Please do feel free to make such a PR, I'm currently a bit swamped with other work.

Thanks for both of your comments.
I have applied most of the suggested changes for quotes to code that I wrote or changed, but kept everything else the same to keep this PR focussed.

Posix doesn't matter if dash in termux supports -G, which it would be otherwise we would be seeing errors. stat external call will have performance costs and if its done, should precheck with -e so that does not affect normal users.

Posix doesn't matter if dash in termux supports -G, which it would be otherwise we would be seeing errors. stat external call will have performance costs and if its done, should precheck with -e so that does not affect normal users.

dash does support test -G, but these aren't dash scripts.
They're #!/bin/sh scripts, which is a symlink to dash by default on Termux.
But we should not be assuming dash if we've specified sh in the shebang.

mksh (e.g. /system/bin/sh) for example may or may not support test -G.
On my phone it seems to support it, but we shouldn't assume that a non-standard option is present.

mksh and termux sh even back in Android 5 support -G, not really an issue.

And this change to convert ~/.termux/shell from symlink to text file will break other stuff including possibly 3rd party scripts.

https://github.com/termux/termux-packages/blob/5a979ce68711aedc3b93fd674e39f084e6f6fc2d/disabled-packages/proot/termux-chroot#L39

And this change to convert ~/.termux/shell from symlink to text file will break other stuff including possibly 3rd party scripts.

That is essentially the breaking change proposed here, which is basically one way to solve these two problems:

• chsh -s ash does not work without this change or some other workaround

  • (because ash would be a symbolic link to busybox and the current way, before this PR, of realpath ~/.termux/shell, attempts to execute "busybox" as the shell instead of "ash", which changes the behavior of the busybox binary back to not behaving as a shell and causes the error: -l: applet not found)
  • Technically, from a certain perspective, another way to work around this could be to install ash by installing a duplicated copy of the entire busybox binary file but just name it "ash" instead of "busybox", but that seems inefficient

• chsh -s /system/bin/sh and/or ln -sf /system/bin/sh ~/.termux/shell is not possible/working without this change or some other workaround

  • (because without this change, the -G argument checks the group ownership of /system/bin/sh directly, and does not allow executing it because that file has different group ownership from the Termux user)
  • Technically, from a certain perspective, another way to work around this is to install another duplicated copy of Mksh using pkg install mksh and chsh -s mksh, but that seems inefficient

If you know of different ways to solve those two problems, without introducing a breaking change, it would be interesting to investigate,

Otherwise, it seems like a good idea to me, which would need to be documented in the release notes, and before deploying a release containing the change, some updates applied to all external softwares that read ~/.termux/shell and expect a symbolic link, to make them able to detect whether ~/.termux/shell is a symbolic link or a configuration file.

I guess other ways to resolve the two problems could be to replace the instance of the -G argument with the -e argument, and replace the instance of realpath ~/.termux/shell with an instance of readlink ~/.termux/shell,

but in my opinion, creating a basic configuration file for setting the active shell would make the way Termux works slightly more familiar to users coming from GNU/Linux, who are used to being able to see the user's current login shell stored in plaintext in GNU/Linux's configuration file, /etc/passwd.

Others might have other opinions, and if these problems are not considered high-priority enough to introduce a breaking change to solve, then of course this PR does not absolutely need to be accepted.

I wasn't aware of readlink, that would still satisfy my usecase if others would rather not put the shell path in the file.

I wasn't aware of readlink, that would still satisfy my usecase if others would rather not put the shell path in the file.

Oh yeah readlink is also an option.
Considering it's part of coreutils it can be assumed to be present.

Busybox issue occurs because symlinks are canonicalized by realpath by default, so yes, can easily be solved with readlink and not pass -f. That way third party tools won't break.

A text file to be consistent with linux distro would make sense if we had a formal format for such a config file, and at least not one named shell. So don't think that is too important. If you really want to replace symlink with a text file, then in chsh, maybe if a symlink already exists, then show a confirmation prompt to user that it will now be a text file and may break third party tools they are using, that should at least notify third party tools of the change and user can accept it.

/system/bin/sh issue can easily be solved with following, can also add a case statement to match all /system paths.

SHELL_FILE=~/.termux/shell
if [ -L "$SHELL_FILE" ]; then
	SHELL_TARGET="$(readlink "$SHELL_FILE")"
	if { [ -G "$SHELL_TARGET" ] || [ "$SHELL_TARGET" == "/system/bin/sh" ]; } && \
			is_executable_file "$SHELL_TARGET"; then
		SHELL="$SHELL_TARGET"
	fi
fi

if [ -z "${SHELL:-}" ]; then
	set_default_shell
fi

Additionally, note that we are using $HOME, and someone may also be running login under sudo, in which case $HOME will be home/.suroot, and ownership will be root for home/.suroot/.termux/shell, assuming chsh was run with sudo too.

That makes sense to me, though I would note after seeing this:

if { [ -G "$SHELL_TARGET" ] || [ "$SHELL_TARGET" == "/system/bin/sh" ]; }

if this is confirmation that part of the purpose of -G is to limit what programs can be set as a shell to acceptable paths, then I wanted to mention that GNU/Linux's approach to a whitelist in this context is that GNU/Linux's chsh command reads a second configuration file, /etc/shells, and simply only allows shells hardcoded in this list. If restricting the available paths is important, then maybe reading the whitelist from lines in @TERMUX_PREFIX@/etc/shells would be reasonable. That way if someone needs to manually add an unknown path, and they edit a file from the termux-tools package to do it, their change wouldn't get automatically erased the next time they upgrade their packages.

There was a situation when someone had a problem when using chsh (I am not sure what exactly the problem was but I believe they eventually used chsh -s mksh and it gave what they wanted), and one of the things they tried was using $PREFIX/etc/shells:

• How to add new a new shell using chsh termux-packages#24395

It is just an idea though, and isn't necessarily how this implementation of chsh should work.

Some GNU/Linux distros have a very minimal default /etc/shells file and add lines to it in postinst scripts,

https://gitlab.archlinux.org/archlinux/packaging/packages/filesystem/-/blob/b50d92111d267e8f4dad98aba7ae713ce300f746/shells

https://gitlab.archlinux.org/archlinux/packaging/packages/fish/-/blob/44b86dff3c671b4e0883d168ff15fb99bc47cbfc/fish.install

other ones have a much larger default /etc/shells file that seems to have a goal of covering all normal possibilities:

https://github.com/gentoo/baselayout/blob/21a5713c09bc10104936e4ec8299953509d472c2/etc/shells

Yes, termux-tools was supposed to be updated and tested along with apt 3.0. I suggested termux-tools be bumped in the same apt pull as well.

I agree with merging to the master branch of termux-tools, but I believe there was a wait where termux-tools in the termux-packages repository should not be updated until apt is about to get updated to 3.0+ at the same time, correct?

• As I've mentioned a couple times in other places termux-tools can be updated ahead of [DO NOT MERGE] [TRACKER] Apt 3.1.x termux-packages#24212 without concern.
  The Deb822 codepath will simply be inactive while the repo sources are still in the legacy format.

Yes, termux-tools was supposed to be updated and tested along with apt 3.0. I suggested termux-tools be bumped in the same apt pull as well.

I'm hoping to get back to the Apt 3 updates this weekend.
If your preference is to update termux-tools at the same time, that can be arranged.
I'd prefer hashing out a schedule for when we want to merge that, as well as a prepared new release of termux-tools so we can include the changes to termux-tools since the last tag in that update.

There are pending changes that were requested.

https://github.com/termux/termux-tools/pull/179/files#diff-4e81c28e599bf4d763b288a000ce3a266a0104beb6f6bf8f901a1c2c6880040cL44

Oh right, that would be the quoting of the $shell variable there, correct?

I think merging together was more to do with final testing of both together before release, I haven't done that for both. Deb builds available in same pull would make things easier. Can bump termux-tools in a separate pull too.

How much work is left for apt3?

I think merging together was more to do with final testing of both together before release, I haven't done that for both. Deb builds available in same pull would make things easier. Can bump termux-tools in a separate pull too.

How much work is left for apt3?

Final re-validation for apt, dpkg, apt-file and sequoia-sqv.
And final decision on how we want to handle the emergency rollback script (that it just occurred to me I haven't committed yet).

E.g what needs to be rolled back, and how are we getting the DEBs for that?
(dpkg and apt should suffice for a rollback but we can potentially restore the backed up repo sources as well)

Quote

And add help suggested above to

Please do leave any thoughts on that in termux/termux-packages#24212, I'm gonna call it a night.
I was gonna sleep 3 hours ago and then I got nerd sniped¹ with a Zig issue and a gawk issue.

Also I wrote like 15 commits related to the update-alternatives system today.

• update-alternatives fixes and improvements. termux-packages#25113

And add help suggested above to

termux-tools/scripts/chsh.in

Line 12 in 7ce1a87

echo "Change the login shell."

Unfortunately, I wasn't able to find where the change to show_usage() was suggested, would it be possible for you to link to it?

I added some more detail on how the program works to the help message, hopefully that was what you meant.

I posted the help you had to add, currently it doesn't include empty path either.

Github doesn't let me copy links to review comments. So posting again.

The shell value must be one of following:
- Empty value to restore default shell.
- Absolute path to shell starting with a '/'.
- Relative path to shell not starting with a '/' relative to '@TERMUX_PREFIX@/bin'.

The shell value must be one of following:
- Empty value to restore default shell.
- Absolute path to shell starting with a '/'.
- Relative path to shell not starting with a '/' relative to '@TERMUX_PREFIX@/bin'.

thank you for posting again - I think there must just be a bug in GitHub preventing us from seeing the previous message where that is, because I expanded every review box here and searched the HTML for those strings, and they don't appear anywhere on this page for me except your most recent comment.

Yeah, I think I forgot to submit review. Sorry!

I have added the help message now. Thanks!

Looks good, thanks a lot! Can merge, I will leave any final testing to robert and tom.

Yup, approve from me. I guess its up to robert now.

Thank you for your contribution. 👍