diff mbox series

rust-cross-canadian: Fix for the linker issues caused by using the shell

Message ID 20220904180946.1954417-1-sundeep.kokkonda@gmail.com
State New
Headers show
Series rust-cross-canadian: Fix for the linker issues caused by using the shell | expand

Commit Message

Sundeep KOKKONDA Sept. 4, 2022, 6:09 p.m. UTC
[Yocto #14892]
This is a fix for YOCTO #14878 patch. When sheband is more than 128 characters the default shell /bin/sh is used to execute the linker instead of SDK shell, which causes problems with LD_LIBRARY_PATH.
With this patch shell usage is avoided.

Signed-off-by: Sundeep KOKKONDA <sundeep.kokkonda@gmail.com>
---
 .../rust/files/target-rust-ccld.c             | 39 +++++++++++++++++++
 .../rust/rust-cross-canadian.inc              | 11 +++---
 2 files changed, 44 insertions(+), 6 deletions(-)
 create mode 100644 meta/recipes-devtools/rust/files/target-rust-ccld.c

Comments

Sundeep KOKKONDA Sept. 4, 2022, 6:12 p.m. UTC | #1
Hello,

Since the issue " https://bugzilla.yoctoproject.org/show_bug.cgi?id=14892 " is sporadically appearing at autobuilder, I could not completely test it on my local machine. This patch is posted here to test it on YP AB machine.

I ensured the test is passed at my local machine.

> 
> SDK testing environment: cortexa57-poky-linux
> RESULTS:
> RESULTS - assimp.BuildAssimp.test_assimp: PASSED (78.38s)
> RESULTS - buildcpio.BuildCpioTest.test_cpio: PASSED (42.25s)
> RESULTS - buildepoxy.EpoxyTest.test_epoxy: PASSED (18.43s)
> RESULTS - buildgalculator.GalculatorTest.test_galculator: PASSED (16.29s)
> RESULTS - buildlzip.BuildLzipTest.test_lzip: PASSED (2.41s)
> RESULTS - gcc.GccCompileTest.test_gcc_compile: PASSED (0.11s)
> RESULTS - gcc.GccCompileTest.test_gpp2_compile: PASSED (0.13s)
> RESULTS - gcc.GccCompileTest.test_gpp_compile: PASSED (0.35s)
> RESULTS - gcc.GccCompileTest.test_make: PASSED (0.11s)
> RESULTS - perl.PerlTest.test_perl: PASSED (0.03s)
> RESULTS - python.Python3Test.test_python3: PASSED (0.04s)
> *RESULTS - rust.RustCompileTest.test_cargo_build: PASSED (0.26s)*
> SUMMARY:
> core-image-sato sdk
> (poky-glibc-x86_64-core-image-sato-cortexa57-qemuarm64-toolchain-4.1+snapshot.sh:environment-setup-cortexa57-poky-linux)
> - Ran 12 tests in 158.803s
> core-image-sato sdk - OK - All required tests passed (successes=12,
> skipped=0, failures=0, errors=0)
> NOTE: Tasks Summary: Attempted 1 tasks of which 0 didn't need to be rerun
> and all succeeded.
> 
> 
> 
> 

Please test the patch on autobuilder and let me know if it is ok.

--
Thanks
Sundeep K.
Richard Purdie Sept. 4, 2022, 9:34 p.m. UTC | #2
On Sun, 2022-09-04 at 23:39 +0530, Sundeep KOKKONDA wrote:
> [Yocto #14892]
> This is a fix for YOCTO #14878 patch. When sheband is more than 128 characters the default shell /bin/sh is used to execute the linker instead of SDK shell, which causes problems with LD_LIBRARY_PATH.
> With this patch shell usage is avoided.
> 
> Signed-off-by: Sundeep KOKKONDA <sundeep.kokkonda@gmail.com>
> ---
>  .../rust/files/target-rust-ccld.c             | 39 +++++++++++++++++++
>  .../rust/rust-cross-canadian.inc              | 11 +++---
>  2 files changed, 44 insertions(+), 6 deletions(-)
>  create mode 100644 meta/recipes-devtools/rust/files/target-rust-ccld.c
> 
> diff --git a/meta/recipes-devtools/rust/files/target-rust-ccld.c b/meta/recipes-devtools/rust/files/target-rust-ccld.c
> new file mode 100644
> index 0000000000..13f06b38f8
> --- /dev/null
> +++ b/meta/recipes-devtools/rust/files/target-rust-ccld.c
> @@ -0,0 +1,39 @@
> +/*
> +*
> +* Copyright (C) 2022      Wind River Systems
> +*
> +* SPDX-License-Identifier: GPL-2.0-only
> +*
> +*/
> +
> +#include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +int main (int argc, char *argv[])
> +{
> +        int i=0;
> +        char cc[1024];
> +        char *cmd[1024];
> +        char *ccargs[2][1024];

I'm a bit concerned about the fixed lengths coded here. What if there
are more than 2 arguments in CC/LD? What if one of the arguments (or
the variable) is longer than 1024?

> +
> +        strcpy(cc,getenv("LD"));

The original code this replaces uses CC?


> +        char * pch;
> +        pch = strtok (cc," ");
> +        while (pch != NULL)
> +        {
> +                strcpy(ccargs+i,pch);
> +                printf ("%s\n",ccargs+i);
> +                pch = strtok (NULL, " ");
> +                i++;
> +        }
> +
> +        strcpy(cmd,ccargs+0);
> +
> +        unsetenv("LD_LIBRARY_PATH");
> +        execv(cmd,argv);
> +

Where is ccargs used here (other than the first entry)?

Cheers,

Richard
Richard Purdie Sept. 4, 2022, 10:01 p.m. UTC | #3
On Sun, 2022-09-04 at 22:34 +0100, Richard Purdie via
lists.openembedded.org wrote:
> On Sun, 2022-09-04 at 23:39 +0530, Sundeep KOKKONDA wrote:
> > [Yocto #14892]
> > This is a fix for YOCTO #14878 patch. When sheband is more than 128 characters the default shell /bin/sh is used to execute the linker instead of SDK shell, which causes problems with LD_LIBRARY_PATH.
> > With this patch shell usage is avoided.
> > 
> > Signed-off-by: Sundeep KOKKONDA <sundeep.kokkonda@gmail.com>
> > ---
> >  .../rust/files/target-rust-ccld.c             | 39 +++++++++++++++++++
> >  .../rust/rust-cross-canadian.inc              | 11 +++---
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> >  create mode 100644 meta/recipes-devtools/rust/files/target-rust-ccld.c
> > 
> > diff --git a/meta/recipes-devtools/rust/files/target-rust-ccld.c b/meta/recipes-devtools/rust/files/target-rust-ccld.c
> > new file mode 100644
> > index 0000000000..13f06b38f8
> > --- /dev/null
> > +++ b/meta/recipes-devtools/rust/files/target-rust-ccld.c
> > @@ -0,0 +1,39 @@
> > +/*
> > +*
> > +* Copyright (C) 2022      Wind River Systems
> > +*
> > +* SPDX-License-Identifier: GPL-2.0-only
> > +*
> > +*/
> > +
> > +#include <string.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +
> > +int main (int argc, char *argv[])
> > +{
> > +        int i=0;
> > +        char cc[1024];
> > +        char *cmd[1024];
> > +        char *ccargs[2][1024];
> 
> I'm a bit concerned about the fixed lengths coded here. What if there
> are more than 2 arguments in CC/LD? What if one of the arguments (or
> the variable) is longer than 1024?
> 
> > +
> > +        strcpy(cc,getenv("LD"));
> 
> The original code this replaces uses CC?
> 
> 
> > +        char * pch;
> > +        pch = strtok (cc," ");
> > +        while (pch != NULL)
> > +        {
> > +                strcpy(ccargs+i,pch);
> > +                printf ("%s\n",ccargs+i);
> > +                pch = strtok (NULL, " ");
> > +                i++;
> > +        }
> > +
> > +        strcpy(cmd,ccargs+0);
> > +
> > +        unsetenv("LD_LIBRARY_PATH");
> > +        execv(cmd,argv);
> > +
> 
> Where is ccargs used here (other than the first entry)?

This was getting over complicated. I'm going to try a different version
of this in master-next which calls the shell wrapper from a C program
which just unsets LD_LIBRARY_PATH. See master-next.

Cheers,

Richard
Sundeep KOKKONDA Sept. 5, 2022, 7:12 a.m. UTC | #4
Hello Richard,

Using CC args causing linker error " unknown executable format " so I called linker directly.
Also, I wrote my initial code without any hardcoded symbols and that test code works in my local machine but when the same code executed in Yocto build environment SIGSEGV faults are reported. And to debug the issue in Yocto build environment the printf/echo outputs are not shown in console/log file. So, I modified my code to check whether this solution works in AB machine or not.

Can you let me know what is the error in AB machine with unsetenv("LD_LIBRARY_PATH"); ? Is it the same relocation error?

Thanks,
Sundeep K.
Richard Purdie Sept. 5, 2022, 11:53 a.m. UTC | #5
On Mon, 2022-09-05 at 00:12 -0700, Sundeep KOKKONDA wrote:
> Using CC args causing linker error "unknown executable format" so I
> called linker directly.
> Also, I wrote my initial code without any hardcoded symbols and that
> test code works in my local machine but when the same code executed
> in Yocto build environment SIGSEGV faults are reported. And to debug
> the issue in Yocto build environment the printf/echo outputs are not
> shown in console/log file. So, I modified my code to check whether
> this solution works in AB machine or not.
> 
> Can you let me know what is the error in AB machine
> with unsetenv("LD_LIBRARY_PATH"); ? Is it the same relocation error?

The issues would be due to the issues I mentioned in reply to your
patch on the mailing list. The fixed length string and number of
arguments issues.

In the interests of moving forward, I put the binary shim in front of
the shell script which can make the environment manipulations more
easily.

https://git.yoctoproject.org/poky/commit/?h=master-next&id=7a1bfb2f800c00d3d2bea9e20e5d31d32f247129

The version of the code above seems to work, I ran some tests
specifically on ubuntu1804.

I added in some comments so we know in future why the code is doing
this.

I wondered if we could make that script MIT license rather than GPL-2.0
just to try and keep our licensing fractionally simpler?

Cheers,

Richard
Randy MacLeod Sept. 6, 2022, 12:21 p.m. UTC | #6
On 2022-09-05 07:53, Richard Purdie wrote:
> On Mon, 2022-09-05 at 00:12 -0700, Sundeep KOKKONDA wrote:
>> Using CC args causing linker error "unknown executable format" so I
>> called linker directly.
>> Also, I wrote my initial code without any hardcoded symbols and that
>> test code works in my local machine but when the same code executed
>> in Yocto build environment SIGSEGV faults are reported. And to debug
>> the issue in Yocto build environment the printf/echo outputs are not
>> shown in console/log file. So, I modified my code to check whether
>> this solution works in AB machine or not.
>>
>> Can you let me know what is the error in AB machine
>> with unsetenv("LD_LIBRARY_PATH"); ? Is it the same relocation error?
> The issues would be due to the issues I mentioned in reply to your
> patch on the mailing list. The fixed length string and number of
> arguments issues.
>
> In the interests of moving forward, I put the binary shim in front of
> the shell script which can make the environment manipulations more
> easily.
>
> https://git.yoctoproject.org/poky/commit/?h=master-next&id=7a1bfb2f800c00d3d2bea9e20e5d31d32f247129
>
> The version of the code above seems to work, I ran some tests
> specifically on ubuntu1804.
>
> I added in some comments so we know in future why the code is doing
> this.
>
> I wondered if we could make that script MIT license rather than GPL-2.0
> just to try and keep our licensing fractionally simpler?
Yes, that should be fine. The rust code is dual-licensed

under Apache 2.0 and MIT  terms so we shouldn't be using GPL here.

Sundeep,
Please resend using an MIT license and Richard's changes linked above.

../Randy


>
> Cheers,
>
> Richard
>
>
>
>
>
>
diff mbox series

Patch

diff --git a/meta/recipes-devtools/rust/files/target-rust-ccld.c b/meta/recipes-devtools/rust/files/target-rust-ccld.c
new file mode 100644
index 0000000000..13f06b38f8
--- /dev/null
+++ b/meta/recipes-devtools/rust/files/target-rust-ccld.c
@@ -0,0 +1,39 @@ 
+/*
+*
+* Copyright (C) 2022      Wind River Systems
+*
+* SPDX-License-Identifier: GPL-2.0-only
+*
+*/
+
+#include <string.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int main (int argc, char *argv[])
+{
+        int i=0;
+        char cc[1024];
+        char *cmd[1024];
+        char *ccargs[2][1024];
+
+        strcpy(cc,getenv("LD"));
+        char * pch;
+        pch = strtok (cc," ");
+        while (pch != NULL)
+        {
+                strcpy(ccargs+i,pch);
+                printf ("%s\n",ccargs+i);
+                pch = strtok (NULL, " ");
+                i++;
+        }
+
+        strcpy(cmd,ccargs+0);
+
+        unsetenv("LD_LIBRARY_PATH");
+        execv(cmd,argv);
+
+return 0;
+}
+
+
diff --git a/meta/recipes-devtools/rust/rust-cross-canadian.inc b/meta/recipes-devtools/rust/rust-cross-canadian.inc
index 7bf75a4712..375c435bff 100644
--- a/meta/recipes-devtools/rust/rust-cross-canadian.inc
+++ b/meta/recipes-devtools/rust/rust-cross-canadian.inc
@@ -7,16 +7,15 @@  LICENSE = "MIT"
 
 MODIFYTOS = "0"
 
+DEPENDS += "binutils-cross-canadian-${TRANSLATED_TARGET_ARCH}"
+
+SRC_URI += "file://target-rust-ccld.c"
+
 # Need to use our SDK's sh here, see #14878
 create_sdk_wrapper () {
         file="$1"
         shift
-
-        cat <<- EOF > "${file}"
-		#!${base_prefix}/bin/sh
-		\$$1 \$@
-		EOF
-
+	${CC} ${WORKDIR}/target-rust-ccld.c -o "${file}"
         chmod +x "$file"
 }