On 11/08/2016 07:56 AM, Michael Paquier wrote:
On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
<tsunakawa.ta...@jp.fujitsu.com> wrote:
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
Things are this way since b15f9b08 that introduced pgwin32_is_service().
Still, by considering what you say, you definitely have a point that if
postgres is started by another service running as Local System logs are
going where they should not. Let's remove the check for LocalSystem but
still check for SE_GROUP_ENABLED.
I did some testing on patch v5, on my Windows 8 VM. I launched cmd as
Administrator, and registered the service with:
pg_ctl register -D data
I.e. without specifying a user. When I start the service with "net
start", it refuses to start, and there are no messages in the event log.
It refuses to start because "data" is not a valid directory, so that's
correct. But the error message about that is lost.
Added some debugging messages to win32_is_service(), and it confirms
that with this patch (v5), win32_is_service() incorrectly returns false,
while unmodified git master returns true, and writes the error message
to the event log.
So, I think we still need the check for Local System.
So, without any refactoring work, isn't the attached patch just but fine?
That seems to work properly for me.
Just taking a look at the patch, I'm sure it will work.
Committer (Heikki?),
v5 is refactored for HEAD, and v6 is for previous releases without refactoring.
I'd like v5 to be applied to at least HEAD, as it removes a lot of unnecessary
code.
+ if (!CheckTokenMembership(NULL, AdministratorsSid, &IsAdministrators) ||
+ !CheckTokenMembership(NULL, PowerUsersSid, &IsPowerUsers))
{
- if ((EqualSid(AdministratorsSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)) ||
- (EqualSid(PowerUsersSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)))
- {
- success = TRUE;
- break;
- }
+ log_error("could not check access token membership: error code %lu\n",
+ GetLastError());
+ exit(1);
}
I just looked more deeply at your refactoring patch, and I didn't know
about CheckTokenMembership()... The whole logic of your patch depends
on it. That's quite a cleanup that you have here. It looks that the
former implementation just had no knowledge of this routine or it
would just have been used.
Yeah, CheckTokenMembership() seems really handy. Let's switch to that,
but without removing the checks for Local System.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
