[Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Wei-Bo, Chen]

Detail bug report in the following url:
https://bugs.launchpad.net/qemu/+bug/1245703

Remove is_dir_maybe macro condition DT_LNK in util/path.c

Signed-off-by: Wei-Bo, Chen <webberapple@gmail.com>
---
 util/path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/path.c b/util/path.c
index 4e4877e..b99e436 100644
--- a/util/path.c
+++ b/util/path.c
@@ -58,7 +58,7 @@ static struct pathelem *new_entry(const char *root,
 #if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
 # define dirent_type(dirent) ((dirent)->d_type)
 # define is_dir_maybe(type) \
-    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
+    ((type) == DT_DIR || (type) == DT_UNKNOWN)
 #else
 # define dirent_type(dirent) (1)
 # define is_dir_maybe(type)  (type)
-- 
2.5.0

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[陳威伯]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

For further explaination of endless loop happen in util/path.c
For example, set the QEMU_LD_PREFIX="/home/apple/i386"
sudo debootstrap --arch=i386 trusty /home/apple/i386
http://archive.ubuntu.com/ubuntu/
magic is a normal dynamic-linkd 32-bit elf.
qemu-i386 -L /home/apple/i386 /home/apple/magic
This command will produce the endless loop.
Because /home/apple/i386/dev/fd will link to /proc/self/fd/
We could use strace to watch the progress
strace qemu-i386 -L /home/apple/i386 /home/apple/magic
openat(AT_FDCWD,
"/home/apple/i386/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/3/dev/fd/5/15/usr/share/doc/libp11-kit0/examples",
O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 48
If one of the QEMU_LD_PREFIX directory entry is symbolic
link, is_dir_maybe in the add_entry will return true.

# define is_dir_maybe(type) \
    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
Then add the path name with add_dir_maybe function.
And then add the path name again and again.
Produce the infinite loop in the directory searching.
So the solution is not to deal with the symbolic link in the is_dir_maybe
macro
Modify the code as following:
# define is_dir_maybe(type) \
    ((type) == DT_DIR || (type) == DT_UNKNOWN)


On Wed, Jan 6, 2016 at 11:21 PM, Wei-Bo, Chen <webberapple@gmail.com> wrote:

> Detail bug report in the following url:
> https://bugs.launchpad.net/qemu/+bug/1245703
>
> Remove is_dir_maybe macro condition DT_LNK in util/path.c
>
> Signed-off-by: Wei-Bo, Chen <webberapple@gmail.com>
> ---
>  util/path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/path.c b/util/path.c
> index 4e4877e..b99e436 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -58,7 +58,7 @@ static struct pathelem *new_entry(const char *root,
>  #if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
>  # define dirent_type(dirent) ((dirent)->d_type)
>  # define is_dir_maybe(type) \
> -    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
> +    ((type) == DT_DIR || (type) == DT_UNKNOWN)
>  #else
>  # define dirent_type(dirent) (1)
>  # define is_dir_maybe(type)  (type)
> --
> 2.5.0
>
>

[-- Attachment #2: Type: text/html, Size: 3184 bytes --]

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[陳威伯]

[-- Attachment #1: Type: text/plain, Size: 7 bytes --]

ping!!

[-- Attachment #2: Type: text/html, Size: 59 bytes --]

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Peter Maydell]

On 11 January 2016 at 18:52, 陳威伯 <webberapple@gmail.com> wrote:
> ping!!

This is on my list to look at. It's a long-standing bug, because
-L was never designed to be pointed at an arbitrary directory tree:
it's supposed to point to a fairly small set of libraries in a
'system root' generated for a cross-compiling toolchain, not
something like a complete filesystem root with a populated
/dev subdirectory.

thanks
-- PMM

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Peter Maydell]

On 6 January 2016 at 15:21, Wei-Bo, Chen <webberapple@gmail.com> wrote:
> Detail bug report in the following url:
> https://bugs.launchpad.net/qemu/+bug/1245703
>
> Remove is_dir_maybe macro condition DT_LNK in util/path.c
>
> Signed-off-by: Wei-Bo, Chen <webberapple@gmail.com>
> ---
>  util/path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/path.c b/util/path.c
> index 4e4877e..b99e436 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -58,7 +58,7 @@ static struct pathelem *new_entry(const char *root,
>  #if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
>  # define dirent_type(dirent) ((dirent)->d_type)
>  # define is_dir_maybe(type) \
> -    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
> +    ((type) == DT_DIR || (type) == DT_UNKNOWN)
>  #else
>  # define dirent_type(dirent) (1)
>  # define is_dir_maybe(type)  (type)
> --
> 2.5.0

This change would be essentially reverting commit 338d80dd353c50b63,
which specifically added support for symbolic links in the directory
structure. So if we applied it we'd be regressing on the problem
that that change was meant to fix.

Richard, git says that commit was one of yours :-)

thanks
-- PMM

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Richard Henderson]

On 01/15/2016 09:53 AM, Peter Maydell wrote:
>> @@ -58,7 +58,7 @@ static struct pathelem *new_entry(const char *root,
>>  #if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
>>  # define dirent_type(dirent) ((dirent)->d_type)
>>  # define is_dir_maybe(type) \
>> -    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
>> +    ((type) == DT_DIR || (type) == DT_UNKNOWN)
>>  #else
>>  # define dirent_type(dirent) (1)
>>  # define is_dir_maybe(type)  (type)
>> --
>> 2.5.0
> 
> This change would be essentially reverting commit 338d80dd353c50b63,
> which specifically added support for symbolic links in the directory
> structure. So if we applied it we'd be regressing on the problem
> that that change was meant to fix.
> 
> Richard, git says that commit was one of yours :-)

Because gcc and qemu have different names for their sysroot trees, and in my
disks, gcc is the "master".  So I normally have

   .../qemu/run/qemu-alpha -> .../gcc/run-cross/alphaev67-linux/sys-root
   .../qemu/run/qemu-arm -> .../gcc/run-cross/arm-linux-gnueabi/sys-root
   .../qemu/run/qemu-sparc -> .../gcc/run-cross/sparc64-linux/sys-root
   .../qemu/run/qemu-sparc64 -> .../gcc/run-cross/sparc64-linux/sys-root

The DT_LNK is required for traversing even the first link.


r~

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Peter Maydell]

On 15 January 2016 at 18:15, Richard Henderson <rth@twiddle.net> wrote:
> On 01/15/2016 09:53 AM, Peter Maydell wrote:
>>> @@ -58,7 +58,7 @@ static struct pathelem *new_entry(const char *root,
>>>  #if defined(DT_DIR) && defined(DT_UNKNOWN) && defined(DT_LNK)
>>>  # define dirent_type(dirent) ((dirent)->d_type)
>>>  # define is_dir_maybe(type) \
>>> -    ((type) == DT_DIR || (type) == DT_UNKNOWN || (type) == DT_LNK)
>>> +    ((type) == DT_DIR || (type) == DT_UNKNOWN)
>>>  #else
>>>  # define dirent_type(dirent) (1)
>>>  # define is_dir_maybe(type)  (type)
>>> --
>>> 2.5.0
>>
>> This change would be essentially reverting commit 338d80dd353c50b63,
>> which specifically added support for symbolic links in the directory
>> structure. So if we applied it we'd be regressing on the problem
>> that that change was meant to fix.
>>
>> Richard, git says that commit was one of yours :-)
>
> Because gcc and qemu have different names for their sysroot trees, and in my
> disks, gcc is the "master".  So I normally have
>
>    .../qemu/run/qemu-alpha -> .../gcc/run-cross/alphaev67-linux/sys-root
>    .../qemu/run/qemu-arm -> .../gcc/run-cross/arm-linux-gnueabi/sys-root
>    .../qemu/run/qemu-sparc -> .../gcc/run-cross/sparc64-linux/sys-root
>    .../qemu/run/qemu-sparc64 -> .../gcc/run-cross/sparc64-linux/sys-root
>
> The DT_LNK is required for traversing even the first link.

Right. So the path.c code is definitely buggy, but this patch
isn't the right way to fix it. It really doesn't behave
sensibly if you point it at a full root fs, but lots of people
want to do that, so it would be nice if it worked...

I think the underlying thing the code is trying to do is
create a sort of union-mount of the real root filesystem and
the directory you point at with -L. We need to do that in a way
that doesn't insist on scanning everything in the -L directory
on startup.

thanks
-- PMM

Re: [Qemu-devel] [RFC] util: Fix QEMU_LD_PREFIX endless loop

[Richard Henderson]

On 01/19/2016 10:15 AM, Peter Maydell wrote:
> On 15 January 2016 at 18:15, Richard Henderson <rth@twiddle.net> wrote:
>>    .../qemu/run/qemu-alpha -> .../gcc/run-cross/alphaev67-linux/sys-root
>>    .../qemu/run/qemu-arm -> .../gcc/run-cross/arm-linux-gnueabi/sys-root
>>    .../qemu/run/qemu-sparc -> .../gcc/run-cross/sparc64-linux/sys-root
>>    .../qemu/run/qemu-sparc64 -> .../gcc/run-cross/sparc64-linux/sys-root
>>
>> The DT_LNK is required for traversing even the first link.
> 
> Right. So the path.c code is definitely buggy, but this patch
> isn't the right way to fix it. It really doesn't behave
> sensibly if you point it at a full root fs, but lots of people
> want to do that, so it would be nice if it worked...
> 
> I think the underlying thing the code is trying to do is
> create a sort of union-mount of the real root filesystem and
> the directory you point at with -L. We need to do that in a way
> that doesn't insist on scanning everything in the -L directory
> on startup.

I'm happy with any fix that lets my setup above work.  ;-)


r~