OK, so culicidae is *still* broken

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&amp;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?

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 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?

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

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.

[1]: https://blogs.msdn.microsoft.com/winsdk/2009/11/30/how-to-disable-address-space-layout-randomization-aslr/

--
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

On 2017-04-20 16:57:03 +0530, Amit Kapila wrote:

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.

That seems like a fairly restricted range (good!), and squares with
memories of reading about this (can't find the reference anymore). Just
using something like 0x00000F0000000000 as the address might work
sufficiently well. What happens if you just hardcode that in the first
MapViewOfFileEx call, and change RandomizedBaseAddress="FALSE" in
src/tools/msvc/VCBuildProject.pm to true?

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:

On 2017-04-20 16:57:03 +0530, Amit Kapila wrote:

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.

That seems like a fairly restricted range (good!), and squares with
memories of reading about this (can't find the reference anymore). Just
using something like 0x00000F0000000000 as the address might work
sufficiently well. What happens if you just hardcode that in the first
MapViewOfFileEx call, and change RandomizedBaseAddress="FALSE" in
src/tools/msvc/VCBuildProject.pm to true?

The material I found about Linux x86_64 addressing explains that the
underlying hardware address space is only 48 bits. Linux chooses to
normalize this by sign-extending into the upper 16 bits, so that
valid 64-bit addresses are
0000000000000000 .. 00007FFFFFFFFFFF
and
FFFF800000000000 .. FFFFFFFFFFFFFFFF

If I'm counting the bits correctly, Windows is choosing to use only
1/16th of the lower half of the available address space, which seems
a bit odd. However, the main point here is that there should be
quite a bit of available daylight, if they will let us use addresses
above 000007FFFFFE0000 at all. I'd be inclined to do our own low-tech
ASLR by using addresses with a random component, say 00000Fxxxxxx0000.

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 Fri, Apr 21, 2017 at 12:55 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-04-20 16:57:03 +0530, Amit Kapila wrote:

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.

That seems like a fairly restricted range (good!), and squares with
memories of reading about this (can't find the reference anymore). Just
using something like 0x00000F0000000000 as the address might work
sufficiently well. What happens if you just hardcode that in the first
MapViewOfFileEx call, and change RandomizedBaseAddress="FALSE" in
src/tools/msvc/VCBuildProject.pm to true?

The server start generates an error:
2017-04-24 12:02:05.771 IST [8940] FATAL: could not create shared
memory segment: error code 87
2017-04-24 12:02:05.771 IST [8940] DETAIL: Failed system call was
MapViewOfFileEx.

Error code 87 means "invalid parameter". Some googling [1]https://support.microsoft.com/en-us/help/125713/common-file-mapping-problems-and-platform-differences indicates
such an error occurs if we pass the out-of-range address to
MapViewOfFileEx. Another possible theory is that we must pass the
address as multiple of the system's memory allocation granularity as
that is expected by specs of MapViewOfFileEx. I can try doing that
but I am not confident that this is the right approach because of
below text mentioned in docs (msdn) of MapViewOfFileEx.
"While it is possible to specify an address that is safe now (not used
by the operating system), there is no guarantee that the address will
remain safe over time. Therefore, it is better to let the operating
system choose the address.".

I have printed the addresses that system automatically picks for
MapViewOfFileEx (3a20000, 3770000, 3520000, 3720000, 3730000) and they
seem to be in a range which I have posted up thread for 64-bit
processes.

Another thing I have tried is to just start the server by setting
RandomizedBaseAddress="TRUE". I have tried about 15-20 times but
could not reproduce the problem related to shared memory attach. We
have tried the same on one of my colleagues (Ashutosh Sharma) machine
as well, there we could see that error once or twice out of many tries
but couldn't get it consistently. I think if the chances of this
problem to occur are so less, then probably the suggestion made by Tom
to retry if we get collision doesn't sound bad.

[1]: https://support.microsoft.com/en-us/help/125713/common-file-mapping-problems-and-platform-differences

--
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