[PATCH] uninative: call patchelf-uninative only when needed

[PATCH] uninative: call patchelf-uninative only when needed

[Martin Jansa]

mke2fs.real, mkfs.ext2.real, mkfs.ext3.real, mkfs.ext4.real are indentical
binary with multiple hardlinks and we end calling patchelf-uninative 4
times even when the interpreter is already set correctly from the build

To avoid corrupted binaries created by patchelf-0.18.0 when set-interpreter
is called multiple times (on some systems like ubuntu-18.04 this leads to
segfaults elsewhere just ldd complains that the executable is no longer
dynamically linked, but doesn't fail when executed).

The issue was reported upstream with mkfs.ext4.real as possible reproducer:
https://github.com/NixOS/patchelf/issues/492#issuecomment-1602862272
alternatively we can revert
https://github.com/NixOS/patchelf/commit/65cdee904431d16668f95d816a495bc35a05a192
and create new uninative release with update patchelf-uninative, but
better to wait a bit for upstream to have a look and possibly backport
proper fix later, until then this change fixes the mkfs.ext4 issues I was
seeing in kirkstone, mickledore, nanbield since uninative-3.9 upgrade, as
reported in:
https://lists.openembedded.org/g/openembedded-core/message/182862

Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
---
 meta/classes-global/uninative.bbclass | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/meta/classes-global/uninative.bbclass b/meta/classes-global/uninative.bbclass
index 366f7ac793..b54acdd542 100644
--- a/meta/classes-global/uninative.bbclass
+++ b/meta/classes-global/uninative.bbclass
@@ -175,7 +175,12 @@ python uninative_changeinterp () {
             if not elf.isDynamic():
                 continue
 
-            os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
-            subprocess.check_output(("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f), stderr=subprocess.STDOUT)
-            os.chmod(f, s[stat.ST_MODE])
+            current = subprocess.check_output(("patchelf-uninative", "--print-interpreter", f), stderr=subprocess.STDOUT).decode('utf-8').split('\n')[0]
+            if current != d.getVar("UNINATIVE_LOADER"):
+                bb.debug(2, "Changing interpreter from %s to %s with %s" % (current, d.getVar("UNINATIVE_LOADER"), ("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f)))
+                os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
+                subprocess.check_output(("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f), stderr=subprocess.STDOUT)
+                os.chmod(f, s[stat.ST_MODE])
+            else:
+                bb.debug(2, "Interpreter was already set to %s in %s" % (d.getVar("UNINATIVE_LOADER"), f))
 }
-- 
2.41.0

Re: [OE-core] [PATCH] uninative: call patchelf-uninative only when needed

[Richard Purdie]

On Fri, 2023-06-23 at 11:33 +0200, Martin Jansa wrote:
> mke2fs.real, mkfs.ext2.real, mkfs.ext3.real, mkfs.ext4.real are indentical
> binary with multiple hardlinks and we end calling patchelf-uninative 4
> times even when the interpreter is already set correctly from the build
> 
> To avoid corrupted binaries created by patchelf-0.18.0 when set-interpreter
> is called multiple times (on some systems like ubuntu-18.04 this leads to
> segfaults elsewhere just ldd complains that the executable is no longer
> dynamically linked, but doesn't fail when executed).
> 
> The issue was reported upstream with mkfs.ext4.real as possible reproducer:
> https://github.com/NixOS/patchelf/issues/492#issuecomment-1602862272
> alternatively we can revert
> https://github.com/NixOS/patchelf/commit/65cdee904431d16668f95d816a495bc35a05a192
> and create new uninative release with update patchelf-uninative, but
> better to wait a bit for upstream to have a look and possibly backport
> proper fix later, until then this change fixes the mkfs.ext4 issues I was
> seeing in kirkstone, mickledore, nanbield since uninative-3.9 upgrade, as
> reported in:
> https://lists.openembedded.org/g/openembedded-core/message/182862
> 
> Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
> ---
>  meta/classes-global/uninative.bbclass | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/meta/classes-global/uninative.bbclass b/meta/classes-global/uninative.bbclass
> index 366f7ac793..b54acdd542 100644
> --- a/meta/classes-global/uninative.bbclass
> +++ b/meta/classes-global/uninative.bbclass
> @@ -175,7 +175,12 @@ python uninative_changeinterp () {
>              if not elf.isDynamic():
>                  continue
>  
> -            os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
> -            subprocess.check_output(("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f), stderr=subprocess.STDOUT)
> -            os.chmod(f, s[stat.ST_MODE])
> +            current = subprocess.check_output(("patchelf-uninative", "--print-interpreter", f), stderr=subprocess.STDOUT).decode('utf-8').split('\n')[0]
> +            if current != d.getVar("UNINATIVE_LOADER"):
> +                bb.debug(2, "Changing interpreter from %s to %s with %s" % (current, d.getVar("UNINATIVE_LOADER"), ("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f)))
> +                os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
> +                subprocess.check_output(("patchelf-uninative", "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f), stderr=subprocess.STDOUT)
> +                os.chmod(f, s[stat.ST_MODE])
> +            else:
> +                bb.debug(2, "Interpreter was already set to %s in %s" % (d.getVar("UNINATIVE_LOADER"), f))
>  }

I know why you've done it this way but ideally we should make patchelf
handle this internally? It would be nice to avoid fork calls if we can
in the long run.

Cheers,

Richard

Re: [OE-core] [PATCH] uninative: call patchelf-uninative only when needed

[Martin Jansa]

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

On Fri, Jun 23, 2023 at 12:18 PM Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Fri, 2023-06-23 at 11:33 +0200, Martin Jansa wrote:
> > mke2fs.real, mkfs.ext2.real, mkfs.ext3.real, mkfs.ext4.real are
> indentical
> > binary with multiple hardlinks and we end calling patchelf-uninative 4
> > times even when the interpreter is already set correctly from the build
> >
> > To avoid corrupted binaries created by patchelf-0.18.0 when
> set-interpreter
> > is called multiple times (on some systems like ubuntu-18.04 this leads to
> > segfaults elsewhere just ldd complains that the executable is no longer
> > dynamically linked, but doesn't fail when executed).
> >
> > The issue was reported upstream with mkfs.ext4.real as possible
> reproducer:
> > https://github.com/NixOS/patchelf/issues/492#issuecomment-1602862272
> > alternatively we can revert
> >
> https://github.com/NixOS/patchelf/commit/65cdee904431d16668f95d816a495bc35a05a192
> > and create new uninative release with update patchelf-uninative, but
> > better to wait a bit for upstream to have a look and possibly backport
> > proper fix later, until then this change fixes the mkfs.ext4 issues I was
> > seeing in kirkstone, mickledore, nanbield since uninative-3.9 upgrade, as
> > reported in:
> > https://lists.openembedded.org/g/openembedded-core/message/182862
> >
> > Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
> > ---
> >  meta/classes-global/uninative.bbclass | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/meta/classes-global/uninative.bbclass
> b/meta/classes-global/uninative.bbclass
> > index 366f7ac793..b54acdd542 100644
> > --- a/meta/classes-global/uninative.bbclass
> > +++ b/meta/classes-global/uninative.bbclass
> > @@ -175,7 +175,12 @@ python uninative_changeinterp () {
> >              if not elf.isDynamic():
> >                  continue
> >
> > -            os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
> > -            subprocess.check_output(("patchelf-uninative",
> "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f),
> stderr=subprocess.STDOUT)
> > -            os.chmod(f, s[stat.ST_MODE])
> > +            current = subprocess.check_output(("patchelf-uninative",
> "--print-interpreter", f),
> stderr=subprocess.STDOUT).decode('utf-8').split('\n')[0]
> > +            if current != d.getVar("UNINATIVE_LOADER"):
> > +                bb.debug(2, "Changing interpreter from %s to %s with
> %s" % (current, d.getVar("UNINATIVE_LOADER"), ("patchelf-uninative",
> "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f)))
> > +                os.chmod(f, s[stat.ST_MODE] | stat.S_IWUSR)
> > +                subprocess.check_output(("patchelf-uninative",
> "--set-interpreter", d.getVar("UNINATIVE_LOADER"), f),
> stderr=subprocess.STDOUT)
> > +                os.chmod(f, s[stat.ST_MODE])
> > +            else:
> > +                bb.debug(2, "Interpreter was already set to %s in %s" %
> (d.getVar("UNINATIVE_LOADER"), f))
> >  }
>
> I know why you've done it this way but ideally we should make patchelf
> handle this internally? It would be nice to avoid fork calls if we can
> in the long run.
>

yes, but that would require new uninative tarball to be built and applied
in master, mickledore, kirkstone, dunfell, so I was waiting for feedback
from upstream first before changing patchelf and using this as work around
to be able to use uninative 3.9 and 4.0 until patchelf is fixed properly.

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