diff mbox

[pacman-dev] libmakepkg: Support file 5.33's application/x-pie-executable

Message ID c6ffa8bb3eea231c36dab87e6051c04b16e8c0e6.1524246376.git.jan.steffens@gmail.com
State Accepted, archived
Headers show

Commit Message

Jan Alexander Steffens April 20, 2018, 5:46 p.m. UTC
file 5.33 introduces a new MIME type "application/x-pie-executable",
which is used for relocatable binaries. makepkg ignored these binaries
and did not attempt to strip them.

Handle the new MIME type like the old "application/x-sharedlib".
Stripping the binaries with --strip-unneeded to keep relocation
information should be the correct thing to do.

file 5.33 also misidentifies actual libraries as PIE executables, so we
didn't strip any shared libraries, either. We now work around this bug.

Signed-off-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>
---
 scripts/libmakepkg/tidy/strip.sh.in | 2 ++
 1 file changed, 2 insertions(+)

Comments

Gabriel C April 22, 2018, 4:21 p.m. UTC | #1
2018-04-20 19:46 GMT+02:00 Jan Alexander Steffens (heftig)
<jan.steffens@gmail.com>:
> file 5.33 introduces a new MIME type "application/x-pie-executable",
> which is used for relocatable binaries. makepkg ignored these binaries
> and did not attempt to strip them.
>
> Handle the new MIME type like the old "application/x-sharedlib".
> Stripping the binaries with --strip-unneeded to keep relocation
> information should be the correct thing to do.
>
> file 5.33 also misidentifies actual libraries as PIE executables, so we
> didn't strip any shared libraries, either. We now work around this bug.
>
> Signed-off-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>
> ---
>  scripts/libmakepkg/tidy/strip.sh.in | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in
> index e20114c0..36d1b89e 100644
> --- a/scripts/libmakepkg/tidy/strip.sh.in
> +++ b/scripts/libmakepkg/tidy/strip.sh.in
> @@ -125,6 +125,8 @@ tidy_strip() {
>                                         esac;;
>                                 *application/x-executable*) # Binaries
>                                         strip_flags="$STRIP_BINARIES";;
> +                               *application/x-pie-executable*)  # Relocatable binaries
> +                                       strip_flags="$STRIP_SHARED";;
>                                 *)
>                                         continue ;;
>                         esac

I don't think that's  enough .. since everything , .so ,
.a ( possible glibc2.7+gcc8 ) and any binary is
now marked  application/x-pie-executable in PIE builds.

So probably something like this is what you wish :

https://github.com/abucodonosor/pacman/commit/f38df66ad39383dbce92cef53d3da7ec19208fed

Regards,

Gabriel C
Jan Alexander Steffens April 22, 2018, 7:53 p.m. UTC | #2
On Sun, Apr 22, 2018, 18:22 Gabriel C <nix.or.die@gmail.com> wrote:

> I don't think that's  enough .. since everything , .so ,
> .a ( possible glibc2.7+gcc8 ) and any binary is
> now marked  application/x-pie-executable in PIE builds.
>
> So probably something like this is what you wish :
>
>
> https://github.com/abucodonosor/pacman/commit/f38df66ad39383dbce92cef53d3da7ec19208fed


No, I think that's wrong. You're also removing the relocation information
from the PIE binaries.
Gabriel C April 23, 2018, 12:44 a.m. UTC | #3
2018-04-22 21:53 GMT+02:00 Jan Alexander Steffens <jan.steffens@gmail.com>:
> On Sun, Apr 22, 2018, 18:22 Gabriel C <nix.or.die@gmail.com> wrote:
>
>> I don't think that's  enough .. since everything , .so ,
>> .a ( possible glibc2.7+gcc8 ) and any binary is
>> now marked  application/x-pie-executable in PIE builds.
>>
>> So probably something like this is what you wish :
>>
>>
>> https://github.com/abucodonosor/pacman/commit/f38df66ad39383dbce92cef53d3da7ec19208fed
>
>
> No, I think that's wrong. You're also removing the relocation information
> from the PIE binaries.

So explain why that is wrong and how is different to what is done
right now .. If you say that is wrong then you say is wrong in general.

Also how this removes realocation informations ?
That would be then broken for <file 5.33 ,right ?

Did you run at least plain 'file' to see what file reports now no matter
you have a shared lib , execuable or a static lib ?

$ file ./zstd
./zstd: ELF 64-bit LSB pie executable x86-64, version 1 (SYSV),
dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for
GNU/Linux 4.4.113,
BuildID[sha1]=ad32b90631979b66db1e61d4cf94ae62f74c7656, not stripped


$ file ./libzstd.so.1.3.4
./libzstd.so.1.3.4: ELF 64-bit LSB pie executable x86-64, version 1
(SYSV), dynamically linked,
BuildID[sha1]=f21d565252953c3557f0a02e125bfc483198b5a9, not stripped

 And now that is *not a pie executable* but a shared lib..
Eli Schwartz April 23, 2018, 1:03 a.m. UTC | #4
On 04/22/2018 08:44 PM, Gabriel C wrote:
>> No, I think that's wrong. You're also removing the relocation information
>> from the PIE binaries.
> 
> So explain why that is wrong and how is different to what is done
> right now .. If you say that is wrong then you say is wrong in general.

Well, uh, the whole point is that he's saying we should preserve the
behavior with the new version of file?

> Also how this removes realocation informations ?
> That would be then broken for <file 5.33 ,right ?
> 
> Did you run at least plain 'file' to see what file reports now no matter
> you have a shared lib , execuable or a static lib ?

Okay???

On older versions of file, these were both treated as boring old
application/x-sharedlib, and now on new versions of file it will be
treated as exciting new application/x-pie-executable, which... has the
exact same behavior as application/x-sharedlib.

Because heftig is trying to preserve the status quo, whereas you're
suggesting we break with our current behavior and start treating pie
executables like any other kind.

All this is literally in the commit message for this patch. So let's not
waste time further discussing it....
If you object to the conceptual idea of using the same strip options on
pie executables *and* shared libs, then by all means tell us why you
object, but leave the file program out of it.

Also do tell us why we've been doing the wrong thing for quite some time
now. ;)
Allan McRae April 25, 2018, 2:20 p.m. UTC | #5
On 23/04/18 01:44, Gabriel C wrote:
> 2018-04-22 21:53 GMT+02:00 Jan Alexander Steffens <jan.steffens@gmail.com>:
>> On Sun, Apr 22, 2018, 18:22 Gabriel C <nix.or.die@gmail.com> wrote:
>>
>>> I don't think that's  enough .. since everything , .so ,
>>> .a ( possible glibc2.7+gcc8 ) and any binary is
>>> now marked  application/x-pie-executable in PIE builds.
>>>
>>> So probably something like this is what you wish :
>>>
>>>
>>> https://github.com/abucodonosor/pacman/commit/f38df66ad39383dbce92cef53d3da7ec19208fed
>>
>>
>> No, I think that's wrong. You're also removing the relocation information
>> from the PIE binaries.
> 
> So explain why that is wrong and how is different to what is done
> right now .. If you say that is wrong then you say is wrong in general.
> 

There are two issues here:

1) file sucks.  It is not distinguishing between PIE binaries and libraries.

Has that been reported upstream or are we just working around a bug in
makepkg?  If this is fixed upstream, the fix I will accept into pacman
will be quite different...


2) Once we can detect binaries/libraries correctly again, the default
STRIP_BINARIES in Arch Linux is not suitable for the "PIE by default"
setup.  makepkg.conf needs changed rather than just hiding that...
(FYI, this currently screwing over the Arch glibc PKGBUILD
which uses $STRIP_BINARIES..).

A
Allan McRae April 25, 2018, 2:56 p.m. UTC | #6
On 26/04/18 00:20, Allan McRae wrote:
> There are two issues here:
> 
> 1) file sucks.  It is not distinguishing between PIE binaries and libraries.
> 
> Has that been reported upstream or are we just working around a bug in
> makepkg?  If this is fixed upstream, the fix I will accept into pacman
> will be quite different...
> 

And replying to my own comment...

In the original patch commit:
"file 5.33 also misidentifies actual libraries as PIE executables"

Is there any actually difference between a library and a binary when
they are built with PIE?  Aren't the elf headers exactly the same? Note,
we have been detecting PIE binaries as application/x-sharedlib with
previous file versions.

So either:
1) original patch is fine, as we need to update the description of
STRIP_SHARED to include all PIE things, or
2) we need a STRIP_PIE variable.

Can someone confirm what we get with static PIE with gcc8+?  This will
make my decision.

Allan
Allan McRae May 12, 2018, 12:57 p.m. UTC | #7
On 21/04/18 03:46, Jan Alexander Steffens (heftig) wrote:
> file 5.33 introduces a new MIME type "application/x-pie-executable",
> which is used for relocatable binaries. makepkg ignored these binaries
> and did not attempt to strip them.
> 
> Handle the new MIME type like the old "application/x-sharedlib".
> Stripping the binaries with --strip-unneeded to keep relocation
> information should be the correct thing to do.
> 
> file 5.33 also misidentifies actual libraries as PIE executables, so we
> didn't strip any shared libraries, either. We now work around this bug.
> 
> Signed-off-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>
> ---


Applied with the following documentation additions:

diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt
index 267dc9e9..5417aa0e 100644
--- a/doc/makepkg.conf.5.txt
+++ b/doc/makepkg.conf.5.txt
@@ -193,8 +193,8 @@ Options
        for details.

 **STRIP_SHARED=**"--strip-unneeded"::
-       Options to be used when stripping shared libraries. See
linkman:strip[1]
-       for details.
+       Options to be used when stripping shared libraries or PIE
executables.
+       See linkman:strip[1] for details.

 **STRIP_STATIC=**"--strip-debug"::
        Options to be used when stripping static libraries. See
linkman:strip[1]



>  scripts/libmakepkg/tidy/strip.sh.in | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in
> index e20114c0..36d1b89e 100644
> --- a/scripts/libmakepkg/tidy/strip.sh.in
> +++ b/scripts/libmakepkg/tidy/strip.sh.in
> @@ -125,6 +125,8 @@ tidy_strip() {
>  					esac;;
>  				*application/x-executable*) # Binaries
>  					strip_flags="$STRIP_BINARIES";;
> +				*application/x-pie-executable*)  # Relocatable binaries
> +					strip_flags="$STRIP_SHARED";;
>  				*)
>  					continue ;;
>  			esac
>
diff mbox

Patch

diff --git a/scripts/libmakepkg/tidy/strip.sh.in b/scripts/libmakepkg/tidy/strip.sh.in
index e20114c0..36d1b89e 100644
--- a/scripts/libmakepkg/tidy/strip.sh.in
+++ b/scripts/libmakepkg/tidy/strip.sh.in
@@ -125,6 +125,8 @@  tidy_strip() {
 					esac;;
 				*application/x-executable*) # Binaries
 					strip_flags="$STRIP_BINARIES";;
+				*application/x-pie-executable*)  # Relocatable binaries
+					strip_flags="$STRIP_SHARED";;
 				*)
 					continue ;;
 			esac