Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

export: simplify distro_binary script #697

Merged
merged 1 commit into from
May 2, 2023
Merged

Conversation

akinomyoga
Copy link
Contributor

In Vanilla OS, I have installed git through apx install git. Then, a script file ~/.local/bin/git is generated. The script file seems to be created by distrobox, specifically, by the shell function generate_script in this file (distrobox-export).

In the script file, I noticed that it creates at least 1 + 6 x (number of arguments) extra processes just to quote the arguments and then unquote them, which seems redundant. If the arguments contain ' or ", the number of processes would increase further. We can simply pass the arguments to the command instead of quoting the arguments by external commands and then again unquoting them by eval.

Or is there any particular background to redundantly quote and unquote the arguments? I noticed #448, which introduced this quoting/unquoting in distrobox-export, but I think the change to distrobox-enter was sufficient in #448. While distrobox-enter needs to generate a command string for later use, distrobox_binary generated by distrobox-export evaluates the quoted arguments in the same place so actually does not need to quote them in the first place.

Even when there is a reason to first quote and then unquote the arguments, eval ${command} is vulnerable because when an argument containing '*' is passed, it can pick up an arbitrary string from the filenames in the current directory through pathname expansions and execute it. Even when this PR would be rejected, at least, the eval should be quoted as eval "${command}". For example,

$ touch "''; echo Arbitrary Command Execution; : ''"
$ git commit -m '*'

Also, we can use exec to run distribox-enter in order to reduce the number of forks.

The current version of the "distrobox_binary" script file creates at
least `1 + 6 x (number of arguments)` extra processes to first quote
and then unquote the arguments, which seems redundant.  If the
arguments contain `'` or `"`, the number of processes would increase
further.  We can simply pass the arguments to the command instead of
quoting the arguments using external commands and then unquote them by
`eval`.

Also, `exec` can be used to run `distribox-enter` in order to reduce
the number of forks.
@89luca89
Copy link
Owner

89luca89 commented May 2, 2023

Hi @akinomyoga !

I really appreciate what you did! Thanks a lot for this PR!

@89luca89 89luca89 merged commit 9fd80f8 into 89luca89:main May 2, 2023
8 checks passed
@akinomyoga akinomyoga deleted the patch-1 branch May 2, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants