savoirfairelinux / cqfd

cqfd helps running commands inside the Docker container configured for your project, keeping the user and working directory the same inside the container
GNU General Public License v3.0
65 stars 31 forks source link

cqfd: fix extra arguments containing whitespaces #122

Closed gportay closed 8 months ago

gportay commented 9 months ago

TL;DR; This builts the list of arguments to give to docker run using a bash array; fixing in the meanwhile an issue if whitespaces have to get preserved in the string set by either the variable CQFD_EXTRA_RUN_ARGS or docker_run_args (whitespace MUST get escaped by a backslash).

cqfd allows to pass extra options to docker-run via either the environment variable CQFD_EXTRA_RUN_ARGS or the build property docker_run_args.

The docker-run option -e/--env sets the specified variable in the container. It has two forms:

The first one sets the variable NAME to VALUE while the second sets the variable NAME from the environment (i.e. the variable NAME has to be set and exported: export NAME=VALUE; docker-run ... -e NAME ...).

Some binaries uses environment variables that are list of whitespace seperated value (and clashes with IFS).

Sadly, cqfd fails to set environment variable to the environment of the container if it contains a whitespace, whatever if the whitespace is escaped or if the value is single/double quoted:

$ CQFD_EXTRA_RUN_ARGS='-e MESSAGE="Hello, World!"'
$ export CQFD_EXTRA_RUN_ARGS
$ cqfd run env
docker: invalid reference format: repository name must be lowercase.
See 'docker run --help'.

Or:

$ CQFD_EXTRA_RUN_ARGS='-e MESSAGE=Hello,\ World!'
$ export CQFD_EXTRA_RUN_ARGS
$ cqfd run env
docker: invalid reference format: repository name must be lowercase.
See 'docker run --help'.

Note: The same result happens if using the variable docker_run_args.

Hopefully, cqfd works correcly if the former form is used (i.e. if the environment is used):

$ MESSAGE="Hello, World!"
$ export MESSAGE
$ CQFD_EXTRA_RUN_ARGS='-e MESSAGE'
$ export CQFD_EXTRA_RUN_ARGS
$ cqfd run env
MESSAGE=Hello, World!
HOSTNAME=061a7b5f146e
SSH_AUTH_SOCK=/home/gportay/.sockets/ssh
PWD=/home/gportay/src/cqfd
HOME=/home/gportay
TERM=xterm
SHLVL=0
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
MAIL=/var/mail/gportay
OLDPWD=/
_=/usr/bin/env

The error output is not explicit; however the reason underneath is the variable CQFD_EXTRA_RUN_ARGS is expanded and its value is split into words using the variable IFS.

Note: The command below give a more explicit hint about the error:

$ bash -x cqfd run env
(...)
+ docker run --privileged -e '"MESSAGE=Hello,' 'World!"' --rm --log-driver=none -v /tmp/tmp.wlj6Tm:/bin/cqfd_launch -v /home/gportay/.ssh:/home/gportay/.ssh -v /home/gportay/src/cqfd:/home/gportay/src/cqfd -e HOME=/home/gportay -ti -v /run/user/1000/ssh-agent.socket:/home/gportay/.sockets/ssh -e SSH_AUTH_SOCK=/home/gportay/.sockets/ssh cqfd_gportay_fooinc_barproject_57d1294 cqfd_launch env docker: invalid reference format: repository name must be lowercase.

It results in the three words below, and this is one to many!

Therefore, docker-run considers 'World!' as the image to use and it fails and complains as it does not match the name standard (the uppercase character W, and and even worse, the illegal character !).

That being said, the error is how the variables are expanded to feed the arguments given to docker-run. In this situation, it is expected to split the string by words but not all of them! And there is no smart or magic thing.

The best practice is to ALWAYS double quote the variable in shell to prevent word splitting (and even globing).

Example:

$ doc_path="/home/$USER/My Documents"
$ cd $doc_path
bash: cd: too many arguments

Important: This best practice restricts to have a SINGLE variable by argument.

That being said, and according to own my experience, the use of bash arrays is the ONLY reliable way to build a command-line (or a part of it at least):

$ extra_arg=(--privileged --rm -ti)
$ extra_arg+=(--env "MESSAGE=Hello, World!")
$ docker run "${extra_arg[@]}" debian:latest env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=dc0e9239f4d7
TERM=xterm
MESSAGE=Hello, World!
HOME=/root

The variables CQFD_EXTRA_RUN_ARGS and docker_run_args are shell strings, and they have to get converted to bash arrays.

The bash's read built-in and its option -a ARRAY reads a line from the standard input, splits it into words and assigns it to ARRAY.

$ read -a ARRAY <<<'-e NAME=Hello,\ World'
$ for i in "${ARRAY[@]}"; do printf "%s\n" "$i"; done
-e
NAME=Hello, World

Note: Since both variables CQFD_EXTRA_RUN_ARGS and docker_run_args are shell strings, they are ruled by the shell interpreter: i.e. the spaces are preserved if they are escaped: "-e MESSAGE=Hello, World"

The string set by either CQFD_EXTRA_RUN_ARGS or docker_run_args is converted to a bash array and it is given double quoted to docker run, fixing the whitespace issue describe above; the whole list of argument is built using a bash array in the meanwhile; and the tests are added; end of the story.

gportay commented 9 months ago

Ping?

joufellasfl commented 8 months ago

Thanks Gaël, pr is now merged.