| Message ID | 20190315175005.3279-1-ao2@ao2.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | configure: disallow spaces and colons in source path | expand |
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
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:]:]"
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.
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
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