Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Tom Lane]

Robert Haas  writes:
> Yeah, random() is the wrong thing.  It should use PostmasterRandom().
> Fixed to do that instead.

I am not very happy about this patch; have you considered the security
implications of what you just did?  If you haven't, I'll tell you:
you just made the postmaster's selection of "random" cancel keys and
password salts a lot more predictable.  Formerly, the srandom() seed
for those depended on both the postmaster start time and the time of
the first connection request, but this change removes the first
connection request from the equation.  If you know the postmaster start
time --- which we will happily tell any asker --- it will not take too
many trials to find the seed that's in use.

I'd be the first to agree that this point is inadequately documented
in the code, but PostmasterRandom should be reserved for its existing
security-related uses, not exposed to the world for (ahem) random other
uses.

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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Robert Haas]

On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila  wrote:
> On Thu, Oct 15, 2015 at 8:35 PM, Dmitry Vasilyev 
> wrote:
>>
>> I think that function dsm_impl_windows() with EACCES error should not
>> do ereport() with FATAL level. It works, but it is likely to make an
>> infinite loop if the user will receive EACCES error.
>>
>
> Currently we are using error level as ERROR for creating dsm during
> postmaster startup which is not right and rather we should use error
> level as LOG.  Can you please try with the attached patch and see
> if the issue is fixed for you.
>
> Another some what related point is currently we are using random()
> function to ensure a unique name for dsm and it seems to me that
> it is always going to generate same number on first invocation (at least
> thats what happening on windows) due to which you are seeing the
> error.

Yeah, random() is the wrong thing.  It should use PostmasterRandom().
Fixed to do that instead.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Amit Kapila]

On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas  wrote:
>
> On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila 
wrote:
> > Another some what related point is currently we are using random()
> > function to ensure a unique name for dsm and it seems to me that
> > it is always going to generate same number on first invocation (at least
> > thats what happening on windows) due to which you are seeing the
> > error.  Another options could be to append current pid or data directory
> > path as we are doing in win32_shmem.c.  I think this could be an
> > optimization which can be done in addition to the fix attached (we can
> > do this as a separate patch as well, if we agreed to do anything).
>
> Maybe we need to be using PostmasterRandom() rather than random() for
> the control segment name.
>

+1.  Though I think it is better to investigate the actual cause before
doing
this.

> But regardless, this patch isn't the right fix.
> dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the
> event of a segment-already-exists condition, without ereporting.
>

Thats right, but in this case, it seems from what is reported, that we are
hitting Access Denied error which on retry seems to disappear (which
ideally shouldn't happen).  It's not clear to me why that is happening.

By reading code, it appears that previously when we get such errors
for main shared memory, we replaced the usage of 'Global\PostreSQL:..'
to 'Global/PostgreSQL:.. (refer GetSharedMemName()), but in case of
dsm we have already taken care of same, so not sure what else could
be reason for Access Denied error.

Dmitry, can we try to see what is the exact value of GetLastError()
when it fails (one way is to check the logs at DEBUG5 level,
_dosmaperr() will print that value or you can modify the code to see it.)


Which Windows version you are using?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Tom Lane]

Amit Kapila  writes:
> On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas  wrote:
>> Maybe we need to be using PostmasterRandom() rather than random() for
>> the control segment name.

> +1.  Though I think it is better to investigate the actual cause before
> doing this.

BackendRun() deliberately prevents that from working.  And it also sets
srandom() to a new value for each subprocess, so that AFAICS this idea
would be a net negative.  If you are seeing duplicate key values getting
selected, the problem is elsewhere.

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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Robert Haas]

On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila  wrote:
> Another some what related point is currently we are using random()
> function to ensure a unique name for dsm and it seems to me that
> it is always going to generate same number on first invocation (at least
> thats what happening on windows) due to which you are seeing the
> error.  Another options could be to append current pid or data directory
> path as we are doing in win32_shmem.c.  I think this could be an
> optimization which can be done in addition to the fix attached (we can
> do this as a separate patch as well, if we agreed to do anything).

Maybe we need to be using PostmasterRandom() rather than random() for
the control segment name.

But regardless, this patch isn't the right fix.
dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the
event of a segment-already-exists condition, without ereporting.  If
it hits any OTHER error, then it should ereport().  In the Windows
implementation, the code that caters to this is here:

if (errno == EEXIST)
{
/*
 * On Windows, when the segment already exists, a handle for the
 * existing segment is returned.  We must close it before
 * returning.  We don't do _dosmaperr here, so errno won't be
 * modified.
 */
CloseHandle(hmap);
return false;
}

Kyotaro Horiguchi's analysis seems to me to be going in about the
right direction.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”

[Amit Kapila]

On Fri, Oct 16, 2015 at 12:16 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
> This is wrong, current code does well for this case. I should
> broke the code during investigating the problem.
>
> > > So, to make the windows version behave as the same,
> > > dsm_impl_windows should return false if GetLastError() ==
> > > ERROR_ALREADY_EXISTS regardless of hmap is valid. The current
> > > behavior is wrong because two or more postmaster *share* the same
> > > DSM segment instead of having their own segments.
>
> > > The patch attached will fix *both of* the problems.
>
> So, the patch fixes only the "Permission denied" case.
>

Why do you think it is bad to display even log for "Permission denied"
case?
It seems all other implementations does the same and I think it is
useful information and we should log it.  Don't you think the patch
sent by me is good enough to fix the reported issue?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com