macOS SIP, next try
Previous discussions on macOS SIP:
/messages/by-id/561E73AB.8060800@gmx.net
/messages/by-id/CA+TgmoYGi5oR8Rvb2-SY1_WEjd76H5gCqSukxFQ66qR7MewWAQ@mail.gmail.com
/messages/by-id/6a4d6124-41f0-756b-0811-c5c5def7ef4b@2ndquadrant.com
Tests that use a temporary installation (i.e., make check) use a
platform-specific environment variable to make programs and libraries
find shared libraries in the temporary installation rather than in the
actual configured installation target directory. On macOS, this is
DYLD_LIBRARY_PATH. Ever since System Integrity Protection (SIP)
arrived, this hasn't worked anymore, because DYLD_LIBRARY_PATH gets
disabled by the system (in somewhat obscure ways). The workaround was
to either disable SIP or to do make install before make check.
The solution here is to use install_name_tool to edit the shared library
path in the binaries (programs and libraries) after installation into
the temporary prefix.
The solution is, I think, correct in principle, but the implementation
is hackish, since it hardcodes the names and versions of the interesting
shared libraries in an unprincipled way. More refinement is needed
here. Ideas welcome.
In order to allow all that, we need to link everything with the option
-headerpad_max_install_names so that there is enough room in the
binaries to put in the temporary path in place of the original one.
(The temporary path is strictly longer than the original one.) This
bloats the binaries, so it might only be appropriate for development
builds and should perhaps be hidden behind some option. Ideas also
welcome here.
The attached patch works for me. You can see that it disables setting
DYLD_LIBRARY_PATH, but the tests still work. Verification on other
system variantions welcome, of course.
Comments welcome. I think this direction is promising, but some details
need to be addressed.
Attachments:
0001-WIP-Fix-temp-install-tests-to-work-with-macOS-SIP.patchtext/plain; charset=UTF-8; name=0001-WIP-Fix-temp-install-tests-to-work-with-macOS-SIP.patch; x-mac-creator=0; x-mac-type=0Download
From 28f2f9fc2234e3d993027fb014f1c85f7b3db6be Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 2 Dec 2020 14:49:53 +0100
Subject: [PATCH] WIP: Fix temp-install tests to work with macOS SIP
Tests that use a temporary installation (i.e., make check) use a
platform-specific environment variable to make programs and libraries
find shared libraries in the temporary installation rather than in the
actual configured installation target directory. On macOS, this is
DYLD_LIBRARY_PATH. Ever since System Integrity Protection (SIP)
arrived, this hasn't worked anymore, because DYLD_LIBRARY_PATH gets
disabled by the system (in somewhat obscure ways). The workaround was
to either disable SIP or to do make install before make check.
The solution here is to use install_name_tool to edit the shared
library path in the binaries (programs and libraries) after
installation into the temporary prefix.
XXX The solution is, I think, correct in principle, but the
implementation is hackish, since it hardcodes the names and versions
of the interesting shared libraries in an unprincipled way.
In order to allow that, we need to link everything with the option
-headerpad_max_install_names so that there is enough room in the
binaries to put in the temporary path in place of the original
one. (The temporary path is strictly longer than the original one.)
This bloats the binaries, so it might only be appropriate for
development builds and should perhaps be hidden behind some option.
---
src/Makefile.global.in | 1 +
src/interfaces/ecpg/test/Makefile | 1 +
src/makefiles/Makefile.darwin | 13 ++++++++++++-
src/test/isolation/Makefile | 3 +++
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 7ca1e9aac5..a2349c814b 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -409,6 +409,7 @@ ifeq ($(MAKELEVEL),0)
$(MKDIR_P) '$(abs_top_builddir)'/tmp_install/log
$(MAKE) -C '$(top_builddir)' DESTDIR='$(abs_top_builddir)'/tmp_install install >'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
$(MAKE) -j1 $(if $(CHECKPREP_TOP),-C $(CHECKPREP_TOP),) checkprep >>'$(abs_top_builddir)'/tmp_install/log/install.log 2>&1
+ $(call temp_install_postprocess,'$(abs_top_builddir)/tmp_install/$(bindir)'/* '$(abs_top_builddir)/tmp_install/$(libdir)'/*.so '$(abs_top_builddir)/tmp_install/$(libdir)'/*.dylib)
endif
endif
endif
diff --git a/src/interfaces/ecpg/test/Makefile b/src/interfaces/ecpg/test/Makefile
index be53b7b94d..468e3076ad 100644
--- a/src/interfaces/ecpg/test/Makefile
+++ b/src/interfaces/ecpg/test/Makefile
@@ -80,6 +80,7 @@ endif
REGRESS_OPTS = --dbname=ecpg1_regression,ecpg2_regression --create-role=regress_ecpg_user1,regress_ecpg_user2 $(EXTRA_REGRESS_OPTS)
check: all
+ $(call temp_install_postprocess,$(patsubst %.pgc,%,$(wildcard compat_informix/*.pgc compat_oracle/*.pgc connect/*.pgc pgtypeslib/*.pgc preproc/*.pgc sql/*.pgc thread/*.pgc)))
$(with_temp_install) ./pg_regress $(REGRESS_OPTS) --temp-instance=./tmp_check $(TEMP_CONF) --bindir= $(pg_regress_locale_flags) $(THREAD) --schedule=$(srcdir)/ecpg_schedule sql/twophase
# Connect to the server using TCP, and add a TCP-specific test.
diff --git a/src/makefiles/Makefile.darwin b/src/makefiles/Makefile.darwin
index b17598f058..def6ef0b00 100644
--- a/src/makefiles/Makefile.darwin
+++ b/src/makefiles/Makefile.darwin
@@ -3,7 +3,18 @@ AROPT = crs
DLSUFFIX = .so
# env var name to use in place of LD_LIBRARY_PATH
-ld_library_path_var = DYLD_LIBRARY_PATH
+#ld_library_path_var = DYLD_LIBRARY_PATH
+
+LDFLAGS += -Wl,-headerpad_max_install_names
+
+define temp_install_postprocess
+ for file in $(1); do \
+ for lib in libpq.5 libecpg.6 libecpg_compat.3 libpgtypes.3; do \
+ install_name_tool -change '$(libdir)'/$$lib.dylib '$(abs_top_builddir)/tmp_install/$(libdir)'/$$lib.dylib $$file || exit; \
+ done; \
+ done
+endef
+
ifdef PGXS
BE_DLLLIBS = -bundle_loader $(bindir)/postgres
diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile
index d23e2cec64..7cb6626b8b 100644
--- a/src/test/isolation/Makefile
+++ b/src/test/isolation/Makefile
@@ -40,6 +40,9 @@ pg_isolation_regress$(X): isolation_main.o pg_regress.o $(WIN32RES)
isolationtester$(X): $(OBJS) | submake-libpq submake-libpgport
$(CC) $(CFLAGS) $^ $(libpq_pgport) $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@
+ifeq ($(PORTNAME), darwin)
+ install_name_tool -change '$(libdir)'/libpq.5.dylib $(abs_top_builddir)/src/interfaces/libpq/libpq.5.dylib $@
+endif
distprep: specparse.c specscanner.c
--
2.29.2
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
The solution here is to use install_name_tool to edit the shared library
path in the binaries (programs and libraries) after installation into
the temporary prefix.
Ah, this indeed seems like a promising direction.
The solution is, I think, correct in principle, but the implementation
is hackish, since it hardcodes the names and versions of the interesting
shared libraries in an unprincipled way. More refinement is needed
here. Ideas welcome.
Yeah, it's pretty hacky. I tried the patch as given, and "make check"
failed with
error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/install_name_tool: can't open file: /Users/tgl/pgsql/tmp_install//Users/tgl/testversion/lib/*.so (No such file or directory)
make: *** [temp-install] Error 1
evidently because there are no files named *.so in the "lib" directory
in my build. I removed '$(abs_top_builddir)/tmp_install/$(libdir)'/*.so
from the "call temp_install_postprocess" invocation, and then it got
through "make temp-install", and most of the core regression tests worked.
But a couple of them failed with
+ERROR: could not load library "/Users/tgl/pgsql/tmp_install/Users/tgl/testversion/lib/postgresql/libpqwalreceiver.so": dlopen(/Users/tgl/pgsql/tmp_install/Users/tgl/testversion/lib/postgresql/libpqwalreceiver.so, 10): Library not loaded: /Users/tgl/testversion/lib/libpq.5.dylib
+ Referenced from: /Users/tgl/pgsql/tmp_install/Users/tgl/testversion/lib/postgresql/libpqwalreceiver.so
+ Reason: image not foundWhat this looks like to me is some confusion about whether "pgsql"
or "postgresql" has been injected into the install path or not.
(This build is using /Users/tgl/testversion as the install --prefix;
I surmise that your path had "pgsql" or "postgres" in it already.)
One plausible way to fix this is to use "find" on the install tree
to locate files to modify, instead of hard-wiring knowledge into
the "call temp_install_postprocess" invocation. A possibly better
way is to arrange to invoke install_name_tool once per installed
executable or shlib, at the time we do "make install" for it.
Not sure how hard the latter approach is given our Makefiles.
Another suggestion, after reading "man install_name_tool", is
that maybe you could avoid having to know the names of the specific
libraries if you didn't use per-library '-change' switches, but
instead installed the test-installation rpath with the -add_rpath
switch. This would be closer to the spirit of the existing test
process anyway.
Aside from removing that annoying requirement, this method would
bound the amount of extra header space needed, so instead of
"-headerpad_max_install_names" we could use "-headerpad N" with
some not-hugely-generous N. That ought to damp down the executable
bloat to the point where we'd not have to worry too much about it.
(Pokes around ... it appears that on my old dinosaur prairiedog,
install_name_tool exists and has the -change switch, but not
anything for rpath. So we'd better investigate how old -add_rpath
is. Maybe there's another way to set rpath that works further back?
At least the -headerpad switches exist there.)
I haven't experimented with any of these ideas, so maybe they won't
work for some reason. But I think we ought to push forward looking
for a solution, because it's getting harder and harder to disable
SIP and still have a useful macOS installation :-(
regards, tom lane
On Dec 2, 2020, at 6:28 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
Previous discussions on macOS SIP:
/messages/by-id/561E73AB.8060800@gmx.net
/messages/by-id/CA+TgmoYGi5oR8Rvb2-SY1_WEjd76H5gCqSukxFQ66qR7MewWAQ@mail.gmail.com
/messages/by-id/6a4d6124-41f0-756b-0811-c5c5def7ef4b@2ndquadrant.com
See also /messages/by-id/18012hGLG6HJ9pQDkHAMYuwQKg@sparkpost.com
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Mark Dilger <mark.dilger@enterprisedb.com> writes:
See also /messages/by-id/18012hGLG6HJ9pQDkHAMYuwQKg@sparkpost.com
Yeah. As I recall from that thread and prior ones, the bottleneck is
really just /bin/sh: something, either the kernel or sh itself, is
clearing out DYLD_LIBRARY_PATH when a shell script starts. (Since
PATH is not reset at the same time, this seems the height of
stupidity/uselessness; but I digress.) But if "make" has a setting
for that variable, the setting *is* successfully passed down to
executed programs. Which implies that make doesn't use the shell while
running stuff, which seems odd. But it gives us a possible escape
hatch.
I experimented a bit, and found that the attached very-quick-hack
patch is enough to get through "make check" and also the isolation
tests. Unsurprisingly, none of the TAP tests work. I experimented
a bit and it looks like "prove" may be failing to pass down
DYLD_LIBRARY_PATH to the test scripts, which would be annoying but
could be worked around (say, by getting TestLib.pm to pull the
value from somewhere).
Based on what I've got here, this avenue probably ends up messier
and more invasive than the install_name_tool avenue, since we'd
likely have to teach quite a few places about explicitly passing
down DYLD_LIBRARY_PATH. It does have the advantage that "make
check" doesn't have to test executables that are different from
the bits we'll really install; but that's probably not worth all
that much. Unless someone can think of a cute way to centralize
the changes some more, I'm inclined to think this isn't the way
forward. But it seemed worth posting the results anyway.
regards, tom lane
Attachments:
dyld-hack.patchtext/x-diff; charset=us-ascii; name=dyld-hack.patchDownload
diff --git a/src/common/exec.c b/src/common/exec.c
index 6cbc820042..4de79f94ef 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -323,8 +323,9 @@ int
find_other_exec(const char *argv0, const char *target,
const char *versionstr, char *retpath)
{
- char cmd[MAXPGPATH];
+ char cmd[MAXPGPATH * 2];
char line[MAXPGPATH];
+ char *dlp = getenv("DYLD_LIBRARY_PATH");
if (find_my_exec(argv0, retpath) < 0)
return -1;
@@ -340,7 +341,11 @@ find_other_exec(const char *argv0, const char *target,
if (validate_exec(retpath) != 0)
return -1;
- snprintf(cmd, sizeof(cmd), "\"%s\" -V", retpath);
+ if (dlp)
+ snprintf(cmd, sizeof(cmd), "DYLD_LIBRARY_PATH=\"%s\" \"%s\" -V",
+ dlp, retpath);
+ else
+ snprintf(cmd, sizeof(cmd), "\"%s\" -V", retpath);
if (!pipe_read_line(cmd, line, sizeof(line)))
return -1;
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 5cfb4c4a49..dbde78608a 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -77,6 +77,7 @@ bool debug = false;
char *inputdir = ".";
char *outputdir = ".";
char *bindir = PGBINDIR;
+char *setlibpath = NULL;
char *launcher = NULL;
static _stringlist *loadextension = NULL;
static int max_connections = 0;
@@ -270,7 +271,8 @@ stop_postmaster(void)
fflush(stderr);
snprintf(buf, sizeof(buf),
- "\"%s%spg_ctl\" stop -D \"%s/data\" -s",
+ "%s\"%s%spg_ctl\" stop -D \"%s/data\" -s",
+ setlibpath,
bindir ? bindir : "",
bindir ? "/" : "",
temp_instance);
@@ -1135,7 +1137,8 @@ psql_command(const char *database, const char *query,...)
/* And now we can build and execute the shell command */
snprintf(psql_cmd, sizeof(psql_cmd),
- "\"%s%spsql\" -X -c \"%s\" \"%s\"",
+ "%s\"%s%spsql\" -X -c \"%s\" \"%s\"",
+ setlibpath,
bindir ? bindir : "",
bindir ? "/" : "",
query_escaped,
@@ -1187,7 +1190,7 @@ spawn_process(const char *cmdline)
*/
char *cmdline2;
- cmdline2 = psprintf("exec %s", cmdline);
+ cmdline2 = psprintf("%sexec %s", setlibpath, cmdline);
execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
fprintf(stderr, _("%s: could not exec \"%s\": %s\n"),
progname, shellprog, strerror(errno));
@@ -2290,6 +2293,15 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
unlimit_core_size();
#endif
+ {
+ char *dlp = getenv("DYLD_LIBRARY_PATH");
+
+ if (dlp)
+ setlibpath = psprintf("DYLD_LIBRARY_PATH=\"%s\" ", dlp);
+ else
+ setlibpath = "";
+ }
+
if (temp_instance)
{
FILE *pg_conf;
@@ -2324,7 +2336,8 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
/* initdb */
header(_("initializing database system"));
snprintf(buf, sizeof(buf),
- "\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1",
+ "%s\"%s%sinitdb\" -D \"%s/data\" --no-clean --no-sync%s%s > \"%s/log/initdb.log\" 2>&1",
+ setlibpath,
bindir ? bindir : "",
bindir ? "/" : "",
temp_instance,
@@ -2397,7 +2410,8 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
* Check if there is a postmaster running already.
*/
snprintf(buf2, sizeof(buf2),
- "\"%s%spsql\" -X postgres <%s 2>%s",
+ "%s\"%s%spsql\" -X postgres <%s 2>%s",
+ setlibpath,
bindir ? bindir : "",
bindir ? "/" : "",
DEVNULL, DEVNULL);
BTW, a couple other things that should be noted here:
* Per observation in a nearby thread, install_name_tool seems to
be provided by Apple's "Command Line Tools", but not by Xcode.
This is also true of bison and flex, but we don't require those
in a build-from-tarball. So relying on install_name_tool would
represent an expansion of our requirements for end-user builds.
I don't think this is a fatal objection, not least because the
CLT are a great deal smaller than Xcode --- so we likely ought to
encourage people to install just the former. But it's a change.
* By chance I came across a previous thread in which someone
suggested use of install_name_tool:
/messages/by-id/CAN-RpxCFWqPXQD8CqP3UBqdMwUgQWLG+y7vQgxQdJR8aiKB89A@mail.gmail.com
I griped in that thread that it didn't help for test executables
that don't get installed, such as isolationtester, because the
hack never got applied to them. Re-reading Peter's patch with
that in mind, I see he replaces the path in isolationtester
in-place, which means that it works in "make check" but will
fail in "make installcheck". So I'm not sure how to cope
with that, but it's a problem.
regards, tom lane
So, we haven't gotten anywhere satisfying with these proposed technical
solutions.
I have since learned that there is a way to disable only the part of SIP
that is relevant for us. This seems like a useful compromise, and it
appears that a number of other open-source projects are following the
same route. I suggest the attached documentation patch and then close
this issue.
Attachments:
0001-doc-Update-information-on-macOS-SIP.patchtext/plain; charset=UTF-8; name=0001-doc-Update-information-on-macOS-SIP.patch; x-mac-creator=0; x-mac-type=0Download
From 7efb0ec3e15f37f9c5e12845aeccd9cd8693c01d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 1 Mar 2021 07:58:17 +0100
Subject: [PATCH] doc: Update information on macOS SIP
On more recent versions of macOS, it is sufficient to disable only a
part of SIP in order to get make check to run before make install.
Document how.
---
doc/src/sgml/installation.sgml | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 66ad4ba938..39adf1f5d9 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -2375,6 +2375,9 @@ <title>macOS</title>
You may or may not wish to also install Xcode.
</para>
+ <sect3 id="installation-notes-macos-sysroot">
+ <title>Sysroot</title>
+
<para>
On recent <productname>macOS</productname> releases, it's necessary to
embed the <quote>sysroot</quote> path in the include switches used to
@@ -2419,6 +2422,10 @@ <title>macOS</title>
to build with a non-Apple compiler, but beware that that case is not
tested or supported by the PostgreSQL developers.
</para>
+ </sect3>
+
+ <sect3 id="installation-notes-macos-sip">
+ <title>System Integrity Protection</title>
<para>
<productname>macOS</productname>'s <quote>System Integrity
@@ -2429,6 +2436,25 @@ <title>macOS</title>
install</literal> before <literal>make check</literal>.
Most PostgreSQL developers just turn off SIP, though.
</para>
+
+ <para>
+ To disable SIP, boot into recovery mode, open a terminal, and run
+<programlisting>
+csrutil disable
+</programlisting>
+ and reboot. In macOS version 10.14 and later, it is sufficient to disable
+ the <quote>Debugging</quote> part of SIP, by running
+<programlisting>
+csrutil enable --without debug
+</programlisting>
+ instead. The status of SIP can be shown using
+<programlisting>
+csrutil status
+</programlisting>
+ Note that that status display does not reflect changes until after a
+ reboot.
+ </para>
+ </sect3>
</sect2>
<sect2 id="installation-notes-mingw">
--
2.30.1
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
I have since learned that there is a way to disable only the part of SIP
that is relevant for us. This seems like a useful compromise, and it
appears that a number of other open-source projects are following the
same route. I suggest the attached documentation patch and then close
this issue.
Hmm, interesting. Where is it documented what this does?
regards, tom lane
On 01.03.21 15:44, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
I have since learned that there is a way to disable only the part of SIP
that is relevant for us. This seems like a useful compromise, and it
appears that a number of other open-source projects are following the
same route. I suggest the attached documentation patch and then close
this issue.Hmm, interesting. Where is it documented what this does?
Not really documented AFAICT, but here is a source:
https://developer.apple.com/forums/thread/17452
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 01.03.21 15:44, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
I have since learned that there is a way to disable only the part of SIP
that is relevant for us. This seems like a useful compromise, and it
appears that a number of other open-source projects are following the
same route. I suggest the attached documentation patch and then close
this issue.
Hmm, interesting. Where is it documented what this does?
Not really documented AFAICT, but here is a source:
https://developer.apple.com/forums/thread/17452
Hmm. So I tried this, ie "csrutil enable --without debug" in the
recovery system, and after rebooting what I see is
$ csrutil status
System Integrity Protection status: unknown (Custom Configuration).
Configuration:
Apple Internal: disabled
Kext Signing: enabled
Filesystem Protections: disabled
Debugging Restrictions: enabled
DTrace Restrictions: enabled
NVRAM Protections: enabled
BaseSystem Verification: enabled
This is an unsupported configuration, likely to break in the future and leave your machine in an unknown state.
$
which is, shall we say, not the set of options the command appeared
to select. It does work, in the sense that "make check" is able
to complete without having an installation tree. But really, Apple
is doing their level best to hang a "here be dragons" sign on this.
I'm not comfortable with recommending it, and I'm about to go
turn it off again, because I have no damn idea what it really does.
regards, tom lane
On 05.03.21 01:36, Tom Lane wrote:
Hmm. So I tried this, ie "csrutil enable --without debug" in the
recovery system, and after rebooting what I see is$ csrutil status
System Integrity Protection status: unknown (Custom Configuration).Configuration:
Apple Internal: disabled
Kext Signing: enabled
Filesystem Protections: disabled
Debugging Restrictions: enabled
DTrace Restrictions: enabled
NVRAM Protections: enabled
BaseSystem Verification: enabledThis is an unsupported configuration, likely to break in the future and leave your machine in an unknown state.
$which is, shall we say, not the set of options the command appeared
to select. It does work, in the sense that "make check" is able
to complete without having an installation tree. But really, Apple
is doing their level best to hang a "here be dragons" sign on this.
Yeah, you'd think --without-debug would make "Debugging Restrictions"
disabled. And it used to do that in older macOS versions. My
assessment is that it actually does work, as you also observed, but that
the status display is somehow displaying things incorrectly.
I'm not comfortable with recommending it, and I'm about to go
turn it off again, because I have no damn idea what it really does.
Okay. Interested users can find it at their own risk in this thread or
similar ones elsewhere.
I'm going to close this CF entry now. This is apparently as far as we
can take it for now.