OK, so culicidae is *still* broken
Per
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-04-15%2004%3A00%3A02
2017-04-15 04:31:21.657 GMT [16792] FATAL: could not reattach to shared memory (key=6280001, addr=0x7f692fece000): Invalid argument
Presumably, this is the same issue we've seen on Windows where the
shmem address range gets overlapped by code loaded at a randomized
address. Is there any real hope of making that work?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On April 14, 2017 9:42:41 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Per
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=culicidae&dt=2017-04-15%2004%3A00%3A022017-04-15 04:31:21.657 GMT [16792] FATAL: could not reattach to
shared memory (key=6280001, addr=0x7f692fece000): Invalid argumentPresumably, this is the same issue we've seen on Windows where the
shmem address range gets overlapped by code loaded at a randomized
address. Is there any real hope of making that work?
Seems to work reasonably regularly on other branches... On phone only, so can't dig into details, but it seems there's some chance involved. Let's see what the next few runs will do. Will crank frequency once home.
Andres
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On April 14, 2017 9:42:41 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:
2017-04-15 04:31:21.657 GMT [16792] FATAL: could not reattach to
shared memory (key=6280001, addr=0x7f692fece000): Invalid argumentPresumably, this is the same issue we've seen on Windows where the
shmem address range gets overlapped by code loaded at a randomized
address. Is there any real hope of making that work?
Seems to work reasonably regularly on other branches... On phone only, so can't dig into details, but it seems there's some chance involved. Let's see what the next few runs will do. Will crank frequency once home.
I poked at this on a Fedora 25 box, and was able to reproduce failures at
a rate of one every half dozen or so runs of the core regression tests,
which seems to about match what is happening on culicidae.
Looking at the postmaster's memory map, it seems that shmem segments
get mapped in the same part of the address space as shared libraries,
ie they all end up in 0x00007Fxxxxxxxxxx. So it's not terribly
surprising that there's a risk of collision with a shared library.
I think what may be the most effective way to proceed is to provide
a way to force the shmem segment to be mapped at a chosen address.
It looks like, at least on x86_64 Linux, mapping shmem at
0x00007E0000000000 would work reliably.
Since we only care about this for testing purposes, I don't think
it has to be done in any very clean or even documented way.
I'm inclined to propose that we put something into sysv_shmem.c
that will check for an environment variable named, say, PG_SHMEM_ADDR,
and if it's set will use the value as the address in the initial
shmat() call. For a bit of extra safety we could do that only in
EXEC_BACKEND builds.
Then you'd just need to add PG_SHMEM_ADDR=0x7E0000000000 to
culicidae's build_env and you'd be good to go.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
I think what may be the most effective way to proceed is to provide
a way to force the shmem segment to be mapped at a chosen address.
It looks like, at least on x86_64 Linux, mapping shmem at
0x00007E0000000000 would work reliably.
Since we only care about this for testing purposes, I don't think
it has to be done in any very clean or even documented way.
I'm inclined to propose that we put something into sysv_shmem.c
that will check for an environment variable named, say, PG_SHMEM_ADDR,
and if it's set will use the value as the address in the initial
shmat() call. For a bit of extra safety we could do that only in
EXEC_BACKEND builds.
Concretely, I propose the attached patch. We'd have to put it into
all supported branches, since culicidae is showing intermittent
"could not reattach to shared memory" failures in all the branches.
regards, tom lane
Attachments:
allow-control-of-shmem-attach-address.patchtext/x-diff; charset=us-ascii; name=allow-control-of-shmem-attach-address.patchDownload
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 77cd8f3..15ae672 100644
*** a/src/backend/port/sysv_shmem.c
--- b/src/backend/port/sysv_shmem.c
*************** static void *
*** 102,109 ****
--- 102,129 ----
InternalIpcMemoryCreate(IpcMemoryKey memKey, Size size)
{
IpcMemoryId shmid;
+ void *requestedAddress = NULL;
void *memAddress;
+ /*
+ * Normally we just pass requestedAddress = NULL to shmat(), allowing the
+ * system to choose where the segment gets mapped. But in an EXEC_BACKEND
+ * build, it's possible for whatever is chosen in the postmaster to not
+ * work for backends, due to variations in address space layout. As a
+ * rather klugy workaround, allow the user to specify the address to use
+ * via setting the environment variable PG_SHMEM_ADDR. (If this were of
+ * interest for anything except debugging, we'd probably create a cleaner
+ * and better-documented way to set it, such as a GUC.)
+ */
+ #ifdef EXEC_BACKEND
+ {
+ char *pg_shmem_addr = getenv("PG_SHMEM_ADDR");
+
+ if (pg_shmem_addr)
+ requestedAddress = (void *) strtoul(pg_shmem_addr, NULL, 0);
+ }
+ #endif
+
shmid = shmget(memKey, size, IPC_CREAT | IPC_EXCL | IPCProtection);
if (shmid < 0)
*************** InternalIpcMemoryCreate(IpcMemoryKey mem
*** 203,209 ****
on_shmem_exit(IpcMemoryDelete, Int32GetDatum(shmid));
/* OK, should be able to attach to the segment */
! memAddress = shmat(shmid, NULL, PG_SHMAT_FLAGS);
if (memAddress == (void *) -1)
elog(FATAL, "shmat(id=%d) failed: %m", shmid);
--- 223,229 ----
on_shmem_exit(IpcMemoryDelete, Int32GetDatum(shmid));
/* OK, should be able to attach to the segment */
! memAddress = shmat(shmid, requestedAddress, PG_SHMAT_FLAGS);
if (memAddress == (void *) -1)
elog(FATAL, "shmat(id=%d) failed: %m", shmid);
On 2017-04-15 16:48:05 -0400, Tom Lane wrote:
I wrote:
I think what may be the most effective way to proceed is to provide
a way to force the shmem segment to be mapped at a chosen address.
It looks like, at least on x86_64 Linux, mapping shmem at
0x00007E0000000000 would work reliably.Since we only care about this for testing purposes, I don't think
it has to be done in any very clean or even documented way.
I'm inclined to propose that we put something into sysv_shmem.c
that will check for an environment variable named, say, PG_SHMEM_ADDR,
and if it's set will use the value as the address in the initial
shmat() call. For a bit of extra safety we could do that only in
EXEC_BACKEND builds.Concretely, I propose the attached patch. We'd have to put it into
all supported branches, since culicidae is showing intermittent
"could not reattach to shared memory" failures in all the branches.
That seems quite reasonable. I'm afraid we're going to have to figure
out something similar, but more robust, for windows soon-ish :/
/* OK, should be able to attach to the segment */ - memAddress = shmat(shmid, NULL, PG_SHMAT_FLAGS); + memAddress = shmat(shmid, requestedAddress, PG_SHMAT_FLAGS);if (memAddress == (void *) -1)
elog(FATAL, "shmat(id=%d) failed: %m", shmid);
As a minor point, it'd probably be good to add addr=%zu, requestedAddress
or such.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
That seems quite reasonable. I'm afraid we're going to have to figure
out something similar, but more robust, for windows soon-ish :/
Why doesn't Windows' ability to map the segment into the new process
before it executes take care of that?
As a minor point, it'd probably be good to add addr=%zu, requestedAddress
or such.
Yeah, I'd come to the same conclusion right after sending that.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-15 17:09:38 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
That seems quite reasonable. I'm afraid we're going to have to figure
out something similar, but more robust, for windows soon-ish :/Why doesn't Windows' ability to map the segment into the new process
before it executes take care of that?
Because of ASLR of the main executable (i.e. something like PIE). It'll
supposedly become harder (as in only running in compatibility modes) if
binaries don't enable that. It's currently disabled somewhere in the VC
project generated. Besides that, it'd also be good for security
purposes if we didn't have to disable PIE (which also prevents it from
being used for the initial backend).
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 17:09:38 -0400, Tom Lane wrote:
Why doesn't Windows' ability to map the segment into the new process
before it executes take care of that?
Because of ASLR of the main executable (i.e. something like PIE).
Not following. Are you saying that the main executable gets mapped into
the process address space immediately, but shared libraries are not?
I wonder whether we could work around that by just destroying the created
process and trying again if we get a collision. It'd be a tad
inefficient, but hopefully collisions wouldn't happen often enough to be a
big problem.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-15 17:24:54 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 17:09:38 -0400, Tom Lane wrote:
Why doesn't Windows' ability to map the segment into the new process
before it executes take care of that?Because of ASLR of the main executable (i.e. something like PIE).
Not following. Are you saying that the main executable gets mapped into
the process address space immediately, but shared libraries are not?
Without PIE/ASLR we can somewhat rely on pgwin32_ReserveSharedMemoryRegion
to find the space that PGSharedMemoryCreate allocated still unoccupied.
If the main binary also uses ASLR, not just libraries/stack/other
mappings, that's not guaranteed to be the case.
But this probably needs somebody with actual windows expertise
commenting.
I wonder whether we could work around that by just destroying the created
process and trying again if we get a collision. It'd be a tad
inefficient, but hopefully collisions wouldn't happen often enough to be a
big problem.
That might work, although it's obviously not pretty. We could also just
default to some out-of-the-way address for MapViewOfFileEx, that might
also work.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 16:48:05 -0400, Tom Lane wrote:
Concretely, I propose the attached patch. We'd have to put it into
all supported branches, since culicidae is showing intermittent
"could not reattach to shared memory" failures in all the branches.
That seems quite reasonable.
Pushed, please update culicidae's settings.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 17:24:54 -0400, Tom Lane wrote:
I wonder whether we could work around that by just destroying the created
process and trying again if we get a collision. It'd be a tad
inefficient, but hopefully collisions wouldn't happen often enough to be a
big problem.
That might work, although it's obviously not pretty. We could also just
default to some out-of-the-way address for MapViewOfFileEx, that might
also work.
Could be. Does Microsoft publish any documentation about the range of
addresses their ASLR uses?
Obviously, any such fix would be a lot more likely to be reliable in
64-bit machines. There's probably not enough daylight to be sure of
making it work in 32-bit Windows, so I suspect we'd need some retry
logic anyway for that case.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-15 17:30:21 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 16:48:05 -0400, Tom Lane wrote:
Concretely, I propose the attached patch. We'd have to put it into
all supported branches, since culicidae is showing intermittent
"could not reattach to shared memory" failures in all the branches.That seems quite reasonable.
Pushed, please update culicidae's settings.
Done, although there were already builds running by the time I got to
it, so there'll be a few unaffected runs. I've increased build
frequency of all branches to be forced once-an-hour, so we can quickly
see how reliable it is. Will turn down Monday or such, if everything
looks good.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-15 14:34:28 -0700, Andres Freund wrote:
On 2017-04-15 17:30:21 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 16:48:05 -0400, Tom Lane wrote:
Concretely, I propose the attached patch. We'd have to put it into
all supported branches, since culicidae is showing intermittent
"could not reattach to shared memory" failures in all the branches.That seems quite reasonable.
Pushed, please update culicidae's settings.
Done, although there were already builds running by the time I got to
it, so there'll be a few unaffected runs. I've increased build
frequency of all branches to be forced once-an-hour, so we can quickly
see how reliable it is. Will turn down Monday or such, if everything
looks good.
Looking through all the branches, it seems to have done the trick - the
previous irregular failures seem to be gone. Nice.
Unless somebody complains, I'll reduce the forced build frequency to
something more normal again.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Apr 16, 2017 at 3:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
On 2017-04-15 17:24:54 -0400, Tom Lane wrote:
I wonder whether we could work around that by just destroying the created
process and trying again if we get a collision. It'd be a tad
inefficient, but hopefully collisions wouldn't happen often enough to be a
big problem.That might work, although it's obviously not pretty. We could also just
default to some out-of-the-way address for MapViewOfFileEx, that might
also work.Could be. Does Microsoft publish any documentation about the range of
addresses their ASLR uses?
I have look around to find some information to see if there is any
such address range which could be used for our purpose. I am not able
to see any such predictable address range. You might want to read the
article [1]https://blogs.msdn.microsoft.com/winsdk/2009/11/30/how-to-disable-address-space-layout-randomization-aslr/ especially the text around "What is the memory address
space range in virtual memory map where system DLLs and user DLLs
could load?" It seems to indicate that there is no such address
unless I have misunderstood it. I don't deny the possibility of
having such an address range, but I could not find any info on the
same.
Obviously, any such fix would be a lot more likely to be reliable in
64-bit machines. There's probably not enough daylight to be sure of
making it work in 32-bit Windows, so I suspect we'd need some retry
logic anyway for that case.
Yeah, that kind of thing can work assuming we don't get conflicts too
often, but it could be possible that conflicts are not reported from
ASLR enabled environments because of commit 7f3e17b4.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sun, Apr 16, 2017 at 3:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Obviously, any such fix would be a lot more likely to be reliable in
64-bit machines. There's probably not enough daylight to be sure of
making it work in 32-bit Windows, so I suspect we'd need some retry
logic anyway for that case.
Yeah, that kind of thing can work assuming we don't get conflicts too
often, but it could be possible that conflicts are not reported from
ASLR enabled environments because of commit 7f3e17b4.
Right, but Andres' point is that we should make an effort to undo that
hack and instead allow ASLR to happen. Not just because it's allegedly
more secure, but because we may have no choice in future Windows versions.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017-04-19 10:15:31 -0400, Tom Lane wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sun, Apr 16, 2017 at 3:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Obviously, any such fix would be a lot more likely to be reliable in
64-bit machines. There's probably not enough daylight to be sure of
making it work in 32-bit Windows, so I suspect we'd need some retry
logic anyway for that case.Yeah, that kind of thing can work assuming we don't get conflicts too
often, but it could be possible that conflicts are not reported from
ASLR enabled environments because of commit 7f3e17b4.Right, but Andres' point is that we should make an effort to undo that
hack and instead allow ASLR to happen. Not just because it's allegedly
more secure, but because we may have no choice in future Windows versions.
FWIW, I think it *also* might make us more secure, because addresses in
the postgres binary won't be predictable anymore. Since most of the
windows binaries are built by one source, that's some advantage on its
own.
- Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Apr 19, 2017 at 9:01 PM, Andres Freund <andres@anarazel.de> wrote:
On 2017-04-19 10:15:31 -0400, Tom Lane wrote:
Amit Kapila <amit.kapila16@gmail.com> writes:
On Sun, Apr 16, 2017 at 3:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Obviously, any such fix would be a lot more likely to be reliable in
64-bit machines. There's probably not enough daylight to be sure of
making it work in 32-bit Windows, so I suspect we'd need some retry
logic anyway for that case.Yeah, that kind of thing can work assuming we don't get conflicts too
often, but it could be possible that conflicts are not reported from
ASLR enabled environments because of commit 7f3e17b4.Right, but Andres' point is that we should make an effort to undo that
hack and instead allow ASLR to happen. Not just because it's allegedly
more secure, but because we may have no choice in future Windows versions.FWIW, I think it *also* might make us more secure, because addresses in
the postgres binary won't be predictable anymore.
Agreed. I have done some further study by using VMMap tool in Windows
and it seems to me that all 64-bit processes use address range
(0000000000010000 ~ 000007FFFFFE0000). I have attached two screen
shots to show the address range (lower range and upper range). You
need to refer the lower half of the window in attached screenshots.
At this stage, I am not completely sure whether we can assume some
address out of this range to use in MapViewOfFileEx. Let me know your
thoughts.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachments:
lower_range.pngimage/png; name=lower_range.pngDownload
�PNG
IHDR @ ��@ iCCPICC Profile H��WT�����H(����^�������%`B���
�]D����(�@ve��**����
�7)��k��s��/w������93 (����sP% r��� ?FRr
��00(0����FG�(c����-������X�:�_E���@�!N��� �`�
�C����|1�XU @��8C�5�8M��$6q1L�} +�X� hb��Bv�Cs��sx|�k �bg�8?��*77bE2�fi����[����,V�8��"�?O�������Kn�hl�2�1��a��f���������(�U ���H���^�(8^f?�2a��: (���� ��X]��+��,��������8N���������pY�����1��+��I��@Wz�83.Q�=W�K���q�0;6L���8�9f#��9A�.]#��4r�cya6l�d.�0����`�/��&��q�p��0�/�����#�-�����c[�9A1�:c����c��p�I��=�b�F��z�_'��� 0�?` li d^�@� �% , ��e�1�D�~cA1�".���IF�����k�_k�.-�xd�g��Z������l��+�6��P��@�'���<��ul��7�0�savb.����#<#tnzwAx*�"���+���"@�(�.
����M k'����!w\���#�����9A��E�����������G��Y��d,�������Q������-����"v���bM������v��������06[��[6���������an�l~q�����f`����22��4�2B�l+���� ��]zt^������7��R &�=�Mq��+ ����3�
�k'