[PATCH] elf: Properly remove the initial 'env' command

[PATCH] elf: Properly remove the initial 'env' command

[H.J. Lu]

tst-rtld-list-diagnostics.py is called by

"$(test-wrapper-env) $(objpfx)$(rtld-installed-name) --list-diagnostics"

and $(test-wrapper-env) is set to "$(test-wrapper) env".  When there is
a test wrapper, it is incorrect to use:

    # Remove the initial 'env' command.
    parse_diagnostics(opts.command.split()[1:])

to remove 'env'.  Change tst-rtld-list-diagnostics.py to explicitly check
'env' instead.  This fixes [BZ #31357].
---
 elf/tst-rtld-list-diagnostics.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
index 9e70e74bf8..dfba94de64 100644
--- a/elf/tst-rtld-list-diagnostics.py
+++ b/elf/tst-rtld-list-diagnostics.py
@@ -294,7 +294,11 @@ def main(argv):
         check_consistency_with_manual(opts.manual)
 
     # Remove the initial 'env' command.
-    parse_diagnostics(opts.command.split()[1:])
+    options = []
+    for o in opts.command.split()[0:]:
+        if o != 'env':
+            options.append(o)
+    parse_diagnostics(options)
 
     if errors:
         sys.exit(1)
-- 
2.43.0

Re: [PATCH] elf: Properly remove the initial 'env' command

[Florian Weimer]

* H. J. Lu:

> index 9e70e74bf8..dfba94de64 100644
> --- a/elf/tst-rtld-list-diagnostics.py
> +++ b/elf/tst-rtld-list-diagnostics.py
> @@ -294,7 +294,11 @@ def main(argv):
>          check_consistency_with_manual(opts.manual)
>  
>      # Remove the initial 'env' command.
> -    parse_diagnostics(opts.command.split()[1:])
> +    options = []
> +    for o in opts.command.split()[0:]:
> +        if o != 'env':
> +            options.append(o)
> +    parse_diagnostics(options)

I think you can write this as:

    options = opts.command.split()[:]
    options.remove('env')

It only removes the first occurrence, but I think that is more correct
anyway.

Thanks,
Florian

Re: [PATCH] elf: Properly remove the initial 'env' command

[H.J. Lu]

On Fri, Feb 9, 2024 at 5:00 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > index 9e70e74bf8..dfba94de64 100644
> > --- a/elf/tst-rtld-list-diagnostics.py
> > +++ b/elf/tst-rtld-list-diagnostics.py
> > @@ -294,7 +294,11 @@ def main(argv):
> >          check_consistency_with_manual(opts.manual)
> >
> >      # Remove the initial 'env' command.
> > -    parse_diagnostics(opts.command.split()[1:])
> > +    options = []
> > +    for o in opts.command.split()[0:]:
> > +        if o != 'env':
> > +            options.append(o)
> > +    parse_diagnostics(options)
>
> I think you can write this as:
>
>     options = opts.command.split()[:]
>     options.remove('env')
>
> It only removes the first occurrence, but I think that is more correct
> anyway.
>

Fixed in v2.

Thanks.

-- 
H.J.

Re: [PATCH] elf: Properly remove the initial 'env' command

[Andreas Schwab]

On Feb 09 2024, H.J. Lu wrote:

> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
> index 9e70e74bf8..dfba94de64 100644
> --- a/elf/tst-rtld-list-diagnostics.py
> +++ b/elf/tst-rtld-list-diagnostics.py
> @@ -294,7 +294,11 @@ def main(argv):
>          check_consistency_with_manual(opts.manual)
>  
>      # Remove the initial 'env' command.
> -    parse_diagnostics(opts.command.split()[1:])
> +    options = []
> +    for o in opts.command.split()[0:]:
> +        if o != 'env':
> +            options.append(o)

Why does it need to do that in the first place?

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] elf: Properly remove the initial 'env' command

[Florian Weimer]

* Andreas Schwab:

> On Feb 09 2024, H.J. Lu wrote:
>
>> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
>> index 9e70e74bf8..dfba94de64 100644
>> --- a/elf/tst-rtld-list-diagnostics.py
>> +++ b/elf/tst-rtld-list-diagnostics.py
>> @@ -294,7 +294,11 @@ def main(argv):
>>          check_consistency_with_manual(opts.manual)
>>  
>>      # Remove the initial 'env' command.
>> -    parse_diagnostics(opts.command.split()[1:])
>> +    options = []
>> +    for o in opts.command.split()[0:]:
>> +        if o != 'env':
>> +            options.append(o)
>
> Why does it need to do that in the first place?

Good question.  I must have copied it from scripts/tst-ld-trace.py.  I
think we can remove the .split() and run the command string with
shell=True.

Thanks,
Florian

Re: [PATCH] elf: Properly remove the initial 'env' command

[H.J. Lu]

On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Andreas Schwab:
>
> > On Feb 09 2024, H.J. Lu wrote:
> >
> >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
> >> index 9e70e74bf8..dfba94de64 100644
> >> --- a/elf/tst-rtld-list-diagnostics.py
> >> +++ b/elf/tst-rtld-list-diagnostics.py
> >> @@ -294,7 +294,11 @@ def main(argv):
> >>          check_consistency_with_manual(opts.manual)
> >>
> >>      # Remove the initial 'env' command.
> >> -    parse_diagnostics(opts.command.split()[1:])
> >> +    options = []
> >> +    for o in opts.command.split()[0:]:
> >> +        if o != 'env':
> >> +            options.append(o)
> >
> > Why does it need to do that in the first place?
>
> Good question.  I must have copied it from scripts/tst-ld-trace.py.  I
> think we can remove the .split() and run the command string with

How does removing .split() work?  The argument is

"/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env
/export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2
--list-diagnostics"

We need .split() to change it to proper arguments.

> shell=True.
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH] elf: Properly remove the initial 'env' command

[Florian Weimer]

* H. J. Lu:

> On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Andreas Schwab:
>>
>> > On Feb 09 2024, H.J. Lu wrote:
>> >
>> >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
>> >> index 9e70e74bf8..dfba94de64 100644
>> >> --- a/elf/tst-rtld-list-diagnostics.py
>> >> +++ b/elf/tst-rtld-list-diagnostics.py
>> >> @@ -294,7 +294,11 @@ def main(argv):
>> >>          check_consistency_with_manual(opts.manual)
>> >>
>> >>      # Remove the initial 'env' command.
>> >> -    parse_diagnostics(opts.command.split()[1:])
>> >> +    options = []
>> >> +    for o in opts.command.split()[0:]:
>> >> +        if o != 'env':
>> >> +            options.append(o)
>> >
>> > Why does it need to do that in the first place?
>>
>> Good question.  I must have copied it from scripts/tst-ld-trace.py.  I
>> think we can remove the .split() and run the command string with
>
> How does removing .split() work?  The argument is
>
> "/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env
> /export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2
> --list-diagnostics"
>
> We need .split() to change it to proper arguments.

Can you test this?

diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
index 9e70e74bf8..024bd8c320 100644
--- a/elf/tst-rtld-list-diagnostics.py
+++ b/elf/tst-rtld-list-diagnostics.py
@@ -222,7 +222,7 @@ else:
 def parse_diagnostics(cmd):
     global errors
     diag_out = subprocess.run(cmd, stdout=subprocess.PIPE, check=True,
-                              universal_newlines=True).stdout
+                              universal_newlines=True, shell=True).stdout
     if diag_out[-1] != '\n':
         print('error: ld.so output does not end in newline')
         errors += 1
@@ -293,8 +293,7 @@ def main(argv):
     if opts.manual:
         check_consistency_with_manual(opts.manual)
 
-    # Remove the initial 'env' command.
-    parse_diagnostics(opts.command.split()[1:])
+    parse_diagnostics(opts.command)
 
     if errors:
         sys.exit(1)

Thanks,
Florian

Re: [PATCH] elf: Properly remove the initial 'env' command

[H.J. Lu]

On Fri, Feb 9, 2024 at 6:53 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Feb 9, 2024 at 5:30 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Andreas Schwab:
> >>
> >> > On Feb 09 2024, H.J. Lu wrote:
> >> >
> >> >> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
> >> >> index 9e70e74bf8..dfba94de64 100644
> >> >> --- a/elf/tst-rtld-list-diagnostics.py
> >> >> +++ b/elf/tst-rtld-list-diagnostics.py
> >> >> @@ -294,7 +294,11 @@ def main(argv):
> >> >>          check_consistency_with_manual(opts.manual)
> >> >>
> >> >>      # Remove the initial 'env' command.
> >> >> -    parse_diagnostics(opts.command.split()[1:])
> >> >> +    options = []
> >> >> +    for o in opts.command.split()[0:]:
> >> >> +        if o != 'env':
> >> >> +            options.append(o)
> >> >
> >> > Why does it need to do that in the first place?
> >>
> >> Good question.  I must have copied it from scripts/tst-ld-trace.py.  I
> >> think we can remove the .split() and run the command string with
> >
> > How does removing .split() work?  The argument is
> >
> > "/export/build/gnu/tools-build/glibc-apx-sde/test-sde.sh env
> > /export/build/gnu/tools-build/glibc-apx-sde/build-x86_64-linux/elf/ld-linux-x86-64.so.2
> > --list-diagnostics"
> >
> > We need .split() to change it to proper arguments.
>
> Can you test this?
>
> diff --git a/elf/tst-rtld-list-diagnostics.py b/elf/tst-rtld-list-diagnostics.py
> index 9e70e74bf8..024bd8c320 100644
> --- a/elf/tst-rtld-list-diagnostics.py
> +++ b/elf/tst-rtld-list-diagnostics.py
> @@ -222,7 +222,7 @@ else:
>  def parse_diagnostics(cmd):
>      global errors
>      diag_out = subprocess.run(cmd, stdout=subprocess.PIPE, check=True,
> -                              universal_newlines=True).stdout
> +                              universal_newlines=True, shell=True).stdout
>      if diag_out[-1] != '\n':
>          print('error: ld.so output does not end in newline')
>          errors += 1
> @@ -293,8 +293,7 @@ def main(argv):
>      if opts.manual:
>          check_consistency_with_manual(opts.manual)
>
> -    # Remove the initial 'env' command.
> -    parse_diagnostics(opts.command.split()[1:])
> +    parse_diagnostics(opts.command)
>
>      if errors:
>          sys.exit(1)
>
> Thanks,
> Florian
>

It works.  Can you check it in?

Thanks.

-- 
H.J.

Re: [PATCH] elf: Properly remove the initial 'env' command

[Florian Weimer]

* H. J. Lu:

> It works.  Can you check it in?

Thanks for checking, pushed.

Florian