Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary preload wrapper in Makefile #15036

Open
p5pRT opened this issue Nov 9, 2015 · 10 comments
Open

Unnecessary preload wrapper in Makefile #15036

p5pRT opened this issue Nov 9, 2015 · 10 comments

Comments

@p5pRT
Copy link

p5pRT commented Nov 9, 2015

Migrated from rt.perl.org#126599 (status was 'open')

Searchable as RT126599$

@p5pRT
Copy link
Author

p5pRT commented Nov 9, 2015

From hanno@hboeck.de

Hi,

The Makefile.SH in perl contains some code that will in certain
situations create a wrapper script called preload used during
compilation.

I think this is
a) not needed
b) doesn't really work how it's intended
c) can cause problems.

The point of the wrapper seems to be to make sure that during the
compilation perl uses the newly compiled libperl and not a libperl that
might be already installed on the system.
However the Makefile is already setting LD_LIBRARY_PATH, so that should
not happen. The LD_LIBRARY_PATH method is less problematic, as I'll
explain below.

The check whether this preload script gets used looks like this​:
  if test -f $archlib/CORE/$libperl; then
This check will only be true if the paths used in the newly compiled
perl are the same as in the one already on the system. E.g. if there's
a libperl on the system with prefix /usr and the newly compiled perl
has prefix /usr/local the check will fail. It won't matter, because as
explained above the whole thing isn't needed anyway.

Finally why I actually found this and why I think it causes problems​: I
tried to compile Perl with Address Sanitizer. This can cause the
compilation to fail if this preload script is created. The reason is
that if you LD_PRELOAD a library that was build with address sanitizer
and then call any executable not built with address sanitizer it will
crash. This caused gcc to crash. This does not happen with the
LD_LIBRARY_PATH, because then only the libraries needed will be
preloaded.

Conclusion​: Please just remove this, I don't think it does any good and
it can cause problems. See attached patch.

--
Hanno Böck
http​://hboeck.de/

mail/jabber​: hanno@​hboeck.de
GPG​: BBB51E42

@p5pRT
Copy link
Author

p5pRT commented Nov 9, 2015

From hanno@hboeck.de

perl-5.22.0-remove-unneeded-preload.diff
--- ./perl-5.22.0/Makefile.SH	2015-05-13 22:19:28.000000000 +0200
+++ ./perl-5.22.0-nopreload/Makefile.SH	2015-10-31 13:45:02.939255468 +0100
@@ -134,26 +134,6 @@
         esac
 
 	case "$osname" in
-	linux)
-	    # If there is a pre-existing $libperl from a previous
-	    # installation, Linux needs to use LD_PRELOAD to
-	    # override the LD_LIBRARY_PATH setting.  See the
-	    # INSTALL file, under "Building a shared perl library".
-	    # If there is no pre-existing $libperl, we don't need
-	    # to do anything further.
-	    if test -f $archlib/CORE/$libperl; then
-		rm -f preload
-		cat <<'EOT' > preload
-#! /bin/sh
-lib=$1
-shift
-test -r $lib && export LD_PRELOAD="$lib $LD_PRELOAD"
-exec "$@"
-EOT
-		chmod 755 preload
-		ldlibpth="$ldlibpth `pwd`/preload `pwd`/$libperl"
-	    fi
-	    ;;
 	os390)	test -f /bin/env && ldlibpth="/bin/env $ldlibpth"
 		;;
 	esac

@p5pRT
Copy link
Author

p5pRT commented Dec 1, 2015

From hanno@hboeck.de

Hi,

I already sent this mail a while ago, but somehow it never got to this
list, so I'm resending it.

The Makefile.SH in perl contains some code that will in certain
situations create a wrapper script called preload used during
compilation.

I think this is
a) not needed
b) doesn't really work how it's intended
c) can cause problems.

The point of the wrapper seems to be to make sure that during the
compilation perl uses the newly compiled libperl and not a libperl that
might be already installed on the system.
However the Makefile is already setting LD_LIBRARY_PATH, so that should
not happen. The LD_LIBRARY_PATH method is less problematic, as I'll
explain below.

The check whether this preload script gets used looks like this​:
  if test -f $archlib/CORE/$libperl; then
This check will only be true if the paths used in the newly compiled
perl are the same as in the one already on the system. E.g. if there's
a libperl on the system with prefix /usr and the newly compiled perl
has prefix /usr/local the check will fail. It won't matter, because as
explained above the whole thing isn't needed anyway.

Finally why I actually found this and why I think it causes problems​: I
tried to compile Perl with Address Sanitizer. This can cause the
compilation to fail if this preload script is created. The reason is
that if you LD_PRELOAD a library that was build with address sanitizer
and then call any executable not built with address sanitizer it will
crash. This caused gcc to crash. This does not happen with the
LD_LIBRARY_PATH, because then only the libraries needed will be
preloaded.

Conclusion​: Please just remove this, I don't think it does any good and
it can cause problems. See attached patch.

--
Hanno Böck
http​://hboeck.de/

mail/jabber​: hanno@​hboeck.de
GPG​: BBB51E42

@p5pRT
Copy link
Author

p5pRT commented Dec 1, 2015

From hanno@hboeck.de

perl-5.22.0-remove-unneeded-preload.diff
--- ./perl-5.22.0/Makefile.SH	2015-05-13 22:19:28.000000000 +0200
+++ ./perl-5.22.0-nopreload/Makefile.SH	2015-10-31 13:45:02.939255468 +0100
@@ -134,26 +134,6 @@
         esac
 
 	case "$osname" in
-	linux)
-	    # If there is a pre-existing $libperl from a previous
-	    # installation, Linux needs to use LD_PRELOAD to
-	    # override the LD_LIBRARY_PATH setting.  See the
-	    # INSTALL file, under "Building a shared perl library".
-	    # If there is no pre-existing $libperl, we don't need
-	    # to do anything further.
-	    if test -f $archlib/CORE/$libperl; then
-		rm -f preload
-		cat <<'EOT' > preload
-#! /bin/sh
-lib=$1
-shift
-test -r $lib && export LD_PRELOAD="$lib $LD_PRELOAD"
-exec "$@"
-EOT
-		chmod 755 preload
-		ldlibpth="$ldlibpth `pwd`/preload `pwd`/$libperl"
-	    fi
-	    ;;
 	os390)	test -f /bin/env && ldlibpth="/bin/env $ldlibpth"
 		;;
 	esac

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

From @jkeenan

On Mon Nov 30 20​:20​:55 2015, hanno@​hboeck.de wrote​:

Hi,

I already sent this mail a while ago, but somehow it never got to this
list, so I'm resending it.

You sent it to perl5-porters-owner@​perl.org, which is not the correct address. It appears that the message lurked in some dark corner of our system until the sysadmin spotted it today and moved it into the Perl 5 bug queue.

The Makefile.SH in perl contains some code that will in certain
situations create a wrapper script called preload used during
compilation.

I think this is
a) not needed
b) doesn't really work how it's intended
c) can cause problems.

The point of the wrapper seems to be to make sure that during the
compilation perl uses the newly compiled libperl and not a libperl that
might be already installed on the system.
However the Makefile is already setting LD_LIBRARY_PATH, so that should
not happen. The LD_LIBRARY_PATH method is less problematic, as I'll
explain below.

The check whether this preload script gets used looks like this​:
if test -f $archlib/CORE/$libperl; then
This check will only be true if the paths used in the newly compiled
perl are the same as in the one already on the system. E.g. if there's
a libperl on the system with prefix /usr and the newly compiled perl
has prefix /usr/local the check will fail. It won't matter, because as
explained above the whole thing isn't needed anyway.

Finally why I actually found this and why I think it causes problems​: I
tried to compile Perl with Address Sanitizer. This can cause the
compilation to fail if this preload script is created. The reason is
that if you LD_PRELOAD a library that was build with address sanitizer
and then call any executable not built with address sanitizer it will
crash. This caused gcc to crash. This does not happen with the
LD_LIBRARY_PATH, because then only the libraries needed will be
preloaded.

Conclusion​: Please just remove this, I don't think it does any good and
it can cause problems. See attached patch.

The patch you attached was drawn against perl-5.22 and so would be unlikely to apply.

I have created this branch​:

smoke-me/jkeenan/126599-preload

... and manually modified Makefile.SH consistent with your request. It is now smoke-testing.

However, I approach this patch with skepticism. The code in question in Makefile.SH has been in place for over 13 years and pertains to linux, which is the tested operating system on which Perl 5 blead is most frequently tested. Hence, if the problem about which you wrote is a significant problem, I would have expected it to have been reported a long time ago.

I'm bcc-ing some of the original contributors to this section of the codebase.

Thanks for the patch.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

From @jkeenan

On Sun Sep 11 19​:23​:54 2016, jkeenan wrote​:

I have created this branch​:

smoke-me/jkeenan/126599-preload

... and manually modified Makefile.SH consistent with your request.
It is now smoke-testing.

For reference, I am attaching the patch found in the smoke branch.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

From @jkeenan

126599-0001-Remove-wrapper-script-preload.patch
From d146d745b3d9e0d62d42242dce0a50c882924be3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Hanno=20B=C3=B6ck?= <hanno@hboeck.de>
Date: Sun, 11 Sep 2016 22:06:47 -0400
Subject: [PATCH] Remove wrapper script "preload".
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Committer: Add Hanno Böck to AUTHORS.

For RT #126599
---
 AUTHORS     |  1 +
 Makefile.SH | 20 --------------------
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index d1b87fc..e923350 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -476,6 +476,7 @@ H.Merijn Brand			<h.m.brand@xs4all.nl>
 Hal Morris			<hom00@utsglobal.com>
 Hal Pomeranz			<pomeranz@netcom.com>
 Hallvard B Furuseth		<h.b.furuseth@usit.uio.no>
+Hanno Böck			<hanno@hboeck.de>
 Hannu Napari			<Hannu.Napari@hut.fi>
 Hans de Graaff			<J.J.deGraaff@twi.tudelft.nl>
 Hans Dieter Pearcey		<hdp@pobox.com>
diff --git a/Makefile.SH b/Makefile.SH
index 42beb81..df2a848 100755
--- a/Makefile.SH
+++ b/Makefile.SH
@@ -143,26 +143,6 @@ true)
         esac
 
 	case "$osname" in
-	linux)
-	    # If there is a pre-existing $libperl from a previous
-	    # installation, Linux needs to use LD_PRELOAD to
-	    # override the LD_LIBRARY_PATH setting.  See the
-	    # INSTALL file, under "Building a shared perl library".
-	    # If there is no pre-existing $libperl, we don't need
-	    # to do anything further.
-	    if test -f $archlib/CORE/$libperl; then
-		rm -f preload
-		cat <<'EOT' > preload
-#! /bin/sh
-lib=$1
-shift
-test -r $lib && export LD_PRELOAD="$lib $LD_PRELOAD"
-exec "$@"
-EOT
-		chmod 755 preload
-		ldlibpth="$ldlibpth `pwd`/preload `pwd`/$libperl"
-	    fi
-	    ;;
 	os390)	test -f /bin/env && ldlibpth="/bin/env $ldlibpth"
 		;;
 	esac
-- 
2.7.4

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

From [Unknown Contact. See original ticket]

On Sun Sep 11 19​:23​:54 2016, jkeenan wrote​:

I have created this branch​:

smoke-me/jkeenan/126599-preload

... and manually modified Makefile.SH consistent with your request.
It is now smoke-testing.

For reference, I am attaching the patch found in the smoke branch.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Sep 12, 2016

From hanno@hboeck.de

Hi,

On Sun, 11 Sep 2016 19​:23​:55 -0700
"James E Keenan via RT" <perlbug-followup@​perl.org> wrote​:

The patch you attached was drawn against perl-5.22 and so would be
unlikely to apply.

I have created this branch​:

smoke-me/jkeenan/126599-preload

... and manually modified Makefile.SH consistent with your request.
It is now smoke-testing.

However, I approach this patch with skepticism. The code in question
in Makefile.SH has been in place for over 13 years and pertains to
linux, which is the tested operating system on which Perl 5 blead is
most frequently tested. Hence, if the problem about which you wrote
is a significant problem, I would have expected it to have been
reported a long time ago.

There already was some follow-up discussion here​:
http​://www.nntp.perl.org/group/perl.perl5.porters/2015/12/msg232991.html
http​://www.nntp.perl.org/group/perl.perl5.porters/2015/12/msg233331.html

However the issue is still unresolved. The reason why this didn't show
up earlier is most likely because ASAN hasn't existed for that long.

I'd still hope we can somehow resolve this and make using ASAN with
perl less problematic.

--
Hanno Böck
https://hboeck.de/

mail/jabber​: hanno@​hboeck.de
GPG​: FE73757FA60E4E21B937579FA5880072BBB51E42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant