configure: disallow spaces and colons in source path

[Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

Posted by Antonio Ospite 5 years, 1 month ago

From: Antonio Ospite <antonio.ospite@collabora.com>

The configure script breaks when the qemu source directory is in a path
containing white spaces, in particular the list of targets is not
correctly generated when calling "./configure --help".

To avoid this issue, refuse to run the configure script if there are
spaces or colons in the source path, this is also what kbuild from linux
does.

Buglink: https://bugs.launchpad.net/qemu/+bug/1817345

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
---
 configure | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure b/configure
index 7071f52584..fbd70a0f51 100755
--- a/configure
+++ b/configure
@@ -295,6 +295,11 @@ libs_qga=""
 debug_info="yes"
 stack_protector=""
 
+if printf "%s" "$(realpath "$source_path")" | grep -q "[[:space:]:]";
+then
+  error_exit "main directory cannot contain spaces nor colons"
+fi
+
 if test -e "$source_path/.git"
 then
     git_update=yes
-- 
2.20.1

Re: [Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

Posted by Peter Maydell 5 years, 1 month ago

On Fri, 15 Mar 2019 at 18:26, Antonio Ospite <ao2@ao2.it> wrote:
>
> From: Antonio Ospite <antonio.ospite@collabora.com>
>
> The configure script breaks when the qemu source directory is in a path
> containing white spaces, in particular the list of targets is not
> correctly generated when calling "./configure --help".
>
> To avoid this issue, refuse to run the configure script if there are
> spaces or colons in the source path, this is also what kbuild from linux
> does.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817345
>
> Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>

Hi Antonio; thanks for this patch.

> ---
>  configure | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/configure b/configure
> index 7071f52584..fbd70a0f51 100755
> --- a/configure
> +++ b/configure
> @@ -295,6 +295,11 @@ libs_qga=""
>  debug_info="yes"
>  stack_protector=""
>
> +if printf "%s" "$(realpath "$source_path")" | grep -q "[[:space:]:]";
> +then
> +  error_exit "main directory cannot contain spaces nor colons"
> +fi
> +

If you do this after the point where we make the source path absolute, you
can skip the realpath (which avoids the problem that 'realpath' doesn't exist
on OSX by default). It will also then be after the handling of the
--source-path option argument.

Do we also need to check for spaces in the path of the build directory
(which is always the current working directory of the script) ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

Posted by Eric Blake 5 years, 1 month ago

On 3/15/19 1:40 PM, Peter Maydell wrote:

> If you do this after the point where we make the source path absolute, you
> can skip the realpath (which avoids the problem that 'realpath' doesn't exist
> on OSX by default). It will also then be after the handling of the
> --source-path option argument.
> 
> Do we also need to check for spaces in the path of the build directory
> (which is always the current working directory of the script) ?

I wasn't thinking about VPATH builds, but yes, in general, we should
ensure that both srcdir and builddir are sane names, while still
allowing symlinks to work around otherwise problematic canonical names.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

Posted by Antonio Ospite 5 years, 1 month ago

Hi, thanks for the comments.

On Fri, 15 Mar 2019 13:48:25 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 3/15/19 1:40 PM, Peter Maydell wrote:
> 
> > If you do this after the point where we make the source path absolute, you
> > can skip the realpath (which avoids the problem that 'realpath' doesn't exist
> > on OSX by default). It will also then be after the handling of the
> > --source-path option argument.
> >
> > Do we also need to check for spaces in the path of the build directory
> > (which is always the current working directory of the script) ?
> 
> I wasn't thinking about VPATH builds, but yes, in general, we should
> ensure that both srcdir and builddir are sane names, while still
> allowing symlinks to work around otherwise problematic canonical names.
> 
[...]

On Fri, 15 Mar 2019 13:46:58 -0500
Eric Blake <eblake@redhat.com> wrote:
>
> Why realpath? If my sources live in "/home/me/bad dir" but I have a
> symlink "/home/me/good", this prevents me from building even though I
> won't trip the problem.
> 

The rationale behind the current patch was that the check should be
done as soon as possible to avoid unneeded work, and since $source_path
is a relative path early on in the script I thought about realpath.

TBH I used realpath unconditionally because I saw it in the Makefile
but I overlooked the fact that it is an internal function in make.

I will move the check after the expansion of $source_path.

After Eric's comment I also tried building from a sane symlink, and
while configure is fine with it "make" still does not like it for
in-tree builds: apparently CURDIR (used to set BUILD_PATH in the
Makefile) resolves symlinks and brings back the bad path.

I do get your point tho, do I did some testing to see the current status
and base the changes on that.

Assuming this setup:

├── no_spaces
│   ├── build
│   ├── qemu
│   └── qemulink -> ../with spaces/qemu
└── with spaces
    ├── build
    ├── qemu
    └── qemulink -> ../no_spaces/qemu

This are the results with the current master branch:

in-tree build:

no_spaces/qemu $ ./configure       # OK
no_spaces/qemu $ make              # OK

no_spaces/qemulink $ ./configure   # OK
no_spaces/qemulink $ make          # FAILS because of CURDIR

with spaces/qemu $ ./configure     # FAILS because of source_path
with spaces/qemu $ make            # FAILS because of SRC_PATH

with spaces/qemulink $ ./configure # FAILS because of source_path
with spaces/qemulink $ make        # FAILS because of SRC_PATH

out-of-tree builds:

no_spaces/build $ ../qemu/configure                 # OK
no_spaces/build $ make                              # OK

no_spaces/build $ ../qemulink/configure             # OK
no_spaces/build $ make                              # OK

no_spaces/build $ ../../with\ spaces/qemu/configure # FAILS
no_spaces/build $ make                              # FAILS no Makefile

no_spaces/build $ ../../with\ spaces/qemulink/configure # FAILS
no_spaces/build $ make                                  # FAILS ^

with spaces/build $ ../qemu/configure               # FAILS
with spaces/build $ make                            # FAILS no Makefile

with spaces/build $ ../qemulink/configure           # FAILS
with spaces/build $ make                            # FAILS no Makefile

with spaces/build $ ../../no_spaces/qemu/configure  # OK
with spaces/build $ make                            # FAILS (CURDIR)

with spaces/build $ ../../no_spaces/qemulink/configure # OK
with spaces/build $ make                               # FAILS (CURDIR)

So checking both source_path and PWD is a probably a good thing.

I'd add the check in the Makefile too, to be on the safe side and cover
the case of: no_spaces/qemulink $ make

Yeah, it is a slow Saturday. :)

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

Posted by Eric Blake 5 years, 1 month ago

On 3/15/19 12:50 PM, Antonio Ospite wrote:
> From: Antonio Ospite <antonio.ospite@collabora.com>
> 
> The configure script breaks when the qemu source directory is in a path
> containing white spaces, in particular the list of targets is not
> correctly generated when calling "./configure --help".
> 
> To avoid this issue, refuse to run the configure script if there are
> spaces or colons in the source path, this is also what kbuild from linux
> does.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1817345
> 
> Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
> ---
>  configure | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/configure b/configure
> index 7071f52584..fbd70a0f51 100755
> --- a/configure
> +++ b/configure
> @@ -295,6 +295,11 @@ libs_qga=""
>  debug_info="yes"
>  stack_protector=""
>  
> +if printf "%s" "$(realpath "$source_path")" | grep -q "[[:space:]:]";

Why realpath? If my sources live in "/home/me/bad dir" but I have a
symlink "/home/me/good", this prevents me from building even though I
won't trip the problem.

Also, grep is not required to operate on non-text files (the POSIX
definition states that if your input does not end in a newline, it is
not a text file, and grep can skip that line) - better is to use "%s\n"
in some form.

So I'd rather see this just use:

if printf %s\\n "$PWD" | grep -q "[[:space:]:]"

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org