Message ID | mvmr1zbrjzc.fsf@suse.de |
---|---|
State | New |
Headers | show |
Series | Use correct exit status in ldd (bug 24150) | expand |
On 2/3/20 6:25 AM, Andreas Schwab wrote: > The message "not a dynamic executable" is not an error, so don't exit with > a nonzero status. I don't consider that a strong enough reason to make this change given the potential for breaking scripts that use ldd. Existing scripts can use ldd to test if a file is a valid dynamic executable, and may be relying on that behaviour. Florian only just had eu-elfclassify added to elfutils to help classify binaries so we could avoid needlessly using ldd for such purposes, but this is relatively new. In summary: - I'd like to see a strong reason to change this. - User scripts should use eu-elfclassify, but may continue to use ldd. > --- > elf/ldd.bash.in | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/elf/ldd.bash.in b/elf/ldd.bash.in > index 467cbf44e9..a879ddd640 100644 > --- a/elf/ldd.bash.in > +++ b/elf/ldd.bash.in > @@ -166,10 +166,7 @@ warning: you do not have execution permission for" "\`$file'" >&2 > case $ret in > 1) > # This can be a non-ELF binary or no binary at all. > - nonelf "$file" || { > - echo $" not a dynamic executable" >&2 > - result=1 > - } > + nonelf "$file" || echo $" not a dynamic executable" If all rtld in ${RTLDLIST} are non-executable this will cause the script to return success when it should not. > ;; > 0|2) > try_trace "$RTLD" "$file" || result=1 >
On Feb 03 2020, Carlos O'Donell wrote: > If all rtld in ${RTLDLIST} are non-executable this will cause the script to return > success when it should not. Why should it not? Andreas.
On 2/3/20 10:58 AM, Andreas Schwab wrote: > On Feb 03 2020, Carlos O'Donell wrote: > >> If all rtld in ${RTLDLIST} are non-executable this will cause the script to return >> success when it should not. > > Why should it not? It is an error if the script has an internal error (lack of executable helper program i.e. ld.so) and cannot carry out the requested user function.
On Feb 03 2020, Carlos O'Donell wrote: > On 2/3/20 10:58 AM, Andreas Schwab wrote: >> On Feb 03 2020, Carlos O'Donell wrote: >> >>> If all rtld in ${RTLDLIST} are non-executable this will cause the script to return >>> success when it should not. >> >> Why should it not? > > It is an error if the script has an internal error (lack of executable helper program i.e. ld.so) and cannot carry out the requested user function. This is not unlike any shared library including ld.so itself where ldd returns sucessful. This case wasn't intended to be an error, otherwise the message would have been formatted as an error. Andreas.
On 2/3/20 11:52 AM, Andreas Schwab wrote: > On Feb 03 2020, Carlos O'Donell wrote: > >> On 2/3/20 10:58 AM, Andreas Schwab wrote: >>> On Feb 03 2020, Carlos O'Donell wrote: >>> >>>> If all rtld in ${RTLDLIST} are non-executable this will cause >>>> the script to return success when it should not. >>> >>> Why should it not? >> >> It is an error if the script has an internal error (lack of >> executable helper program i.e. ld.so) and cannot carry out the >> requested user function. > > This is not unlike any shared library including ld.so itself where > ldd returns sucessful. This case wasn't intended to be an error, > otherwise the message would have been formatted as an error. We should derive what is or is not an error from first principles based on a desire to maintain compatibility with existing scripts and to do what is logical for ldd. If all rtld in ${RTLDLIST} are non-executable then I think we should return a non-zero exit code. We did not do what the user asked and so this is a failure. It is also a relatively obscure corner case so I am not that worried. It is less clear what should happen in the case of your patch where the user-requested file is not something that ldd can process and so there is nothing to do. In that case I'm arguing that we should have a strong reason to change this since it has the potential to impact existing scripts written to use ldd.
diff --git a/elf/ldd.bash.in b/elf/ldd.bash.in index 467cbf44e9..a879ddd640 100644 --- a/elf/ldd.bash.in +++ b/elf/ldd.bash.in @@ -166,10 +166,7 @@ warning: you do not have execution permission for" "\`$file'" >&2 case $ret in 1) # This can be a non-ELF binary or no binary at all. - nonelf "$file" || { - echo $" not a dynamic executable" >&2 - result=1 - } + nonelf "$file" || echo $" not a dynamic executable" ;; 0|2) try_trace "$RTLD" "$file" || result=1