Ultimate-Hosts-Blacklist / Ultimate.Hosts.Blacklist

The Ultimate Unified Hosts file for protecting your network, computer, smartphones and Wi-Fi devices against millions of bad web sites. Protect your children and family from gaining access to bad web sites and protect your devices and pc from being infected with Malware or Ransomware.
MIT License
1.24k stars 155 forks source link

Try harder to become root and speed up by skipping unnecessary sudo's #608

Open ngaro opened 3 years ago

ngaro commented 3 years ago

All other scripts in Installer-Linux can be improved in the same way as below, but I'm not spending time on it if this doesn't get merged...

This PR:

*The 'real' part of the script hasn't been improved yet, that's for another PR. So if one these commands fail you will obviously not see the correct reason yet

rusty-snake commented 3 years ago
  • Speeds up the script because you don't need to run sudo anymore if you are already root

I don't think performance matters for shell-scripts (they have no (JIT-)compiler and start new processes for everything they do). Anyway it allows to run the script on systems without sudo / su.

  • It checks where sudo is located instead of assuming it is in $PATH

To quote man 1 which:

Which takes one or more arguments. For each of its arguments it prints to stdout the full path of the executables that would have been executed when this argument had been entered at the shell prompt. It does this by searching for an executable or script in the directories listed in the environment variable PATH using the same algorithm as bash(1).

So it just searches for it in $PATH.

which on the other hand is also a shell-buildin that even shells as tiny as busybox have

It's not listed in man 1 builtins and LC_ALL=en_US.UTF8 builtin which shows bash: builtin: which: not a shell builtin. command on the other hand is a builtin (at least in bash).

  • It tries to use su if you don't have sudo and warns you that this is deprecated

It is deprecated? man 1 su does not say this, from where is this info?

  • It quits and warns you that you'll have to become root manually if both sudo and su aren't available

We could also check pkexec if we want.

ngaro commented 3 years ago
  • Speeds up the script because you don't need to run sudo anymore if you are already root

I don't think performance matters for shell-scripts (they have no (JIT-)compiler and start new processes for everything they do). Anyway it allows to run the script on systems without sudo / su.

I agree with performance not being important. Still, low-importance improvements are also improvements. Although every call to a binary (e.g. foobar) starts at least 1 process, sudo foobar and su -c foobar start at least 2 processes.

  • It checks where sudo is located instead of assuming it is in $PATH

To quote man 1 which:

Which takes one or more arguments. For each of its arguments it prints to stdout the full path of the executables that would have been executed when this argument had been entered at the shell prompt. It does this by searching for an executable or script in the directories listed in the environment variable PATH using the same algorithm as bash(1).

So it just searches for it in $PATH.

which on the other hand is also a shell-buildin that even shells as tiny as busybox have

It's not listed in man 1 builtins and LC_ALL=en_US.UTF8 builtin which shows bash: builtin: which: not a shell builtin. command on the other hand is a builtin (at least in bash).

It seems that i was a bit to hasty with my conclusion. I tried my code on the tiniest "system" i had (a alpine docker container that barely has any binaries) and because it was present i incorrectly assumed it was a builtin. Sorry about that. Your conclusions are correct.

While writing this I'm suddenly thinking about a better way to check if sudo (or su) is present that doesn't require which or env. I'll fix in my next commit

  • It tries to use su if you don't have sudo and warns you that this is deprecated

It is deprecated? man 1 su does not say this, from where is this info?

True, officially it's not deprecated. But in practice it is. All recent tutorials, books, ... use sudo. Probably because it does everything su can do with the advantage of working correctly on systems without a root password (which are extremely common nowadays). I'll change the warning in the next commit.

  • It quits and warns you that you'll have to become root manually if both sudo and su aren't available

We could also check pkexec if we want.

The name vaguely rings a bell, but I've never used it. I'll take a look at it and use it in a 3rd commit if it it's a extra feature.

rusty-snake commented 3 years ago

While writing this I'm suddenly thinking about a better way to check if sudo (or su) is present that doesn't require which or env.

If you want to only use builtins (is echo always a builtin?):

if ! command -v sudo >&-; then
    echo "no sudo"
fi
ngaro commented 3 years ago

Changes after this commit:

I've tested the returncode 127-rule in bash, zsh and busybox so it's safe to assume that it works everywhere. This doesn't stop developers of choosing to exit their code with 127 themselves but this isn't the case for sudo and su. (And it's probably never the case in well-known programs because it's just 'evil')

Note: Returncode 127 only checks for shell builtins and binaries in $PATH so it could be that the binaries are at a unexpected place and we are not using them although we could have been. But checking this would be extremely slow because commands like locate or find would be necessary. Also, a sudo (or su) at a 'strange' place could just as well be a completely different command that just happens to have the same name.

ngaro commented 3 years ago

According to it's man-page pkexec can return 127 which breaks using 127 to check for a non-existent pkexec :unamused: So in case of a problem I'm just checking for 126 (authorization-related problems) or any other non-zero code (all other problems)

We could extend the code even more by:

But according to me that's overkill, certainly because unless you have a really strange configuration, you will see a 'command-not-found'-type of error anyway and it will cause running which or env which will be a slowdown.

funilrys commented 3 years ago

Hi everyone, this looks marvelous. About your "We could extend the code [...]" part, I agree that it's overkill. People who manage "strange configuration" should be able to handle such thing.

Anything to add @rusty-snake ?

cc @mitchellkrogza.

funilrys commented 3 years ago

Great. Thanks for your input and time @rusty-snake. This PR will stay a draft until the recommendation of @rusty-snake are applied.

Stay safe and healthy.