Re: [HACKERS] Patch to allow users to kill their own queries

2012-01-15 Thread Magnus Hagander
On Fri, Jan 13, 2012 at 14:46, Magnus Hagander  wrote:
> On Fri, Jan 13, 2012 at 14:42, Greg Smith  wrote:
>> On 01/03/2012 12:59 PM, Tom Lane wrote:
>>>
>>> Noah Misch  writes:
>>>

 Regarding the other message, avoid composing a translated message from
 independently-translated parts.

>>>
>>> Yes.  I haven't looked at the patch, but I wonder whether it wouldn't be
>>> better to dodge both of these problems by having the subroutine return a
>>> success/failure result code, so that the actual ereport can be done at
>>> an outer level where the full message (and hint) can be written
>>> directly.
>>>
>>
>>
>> Between your and Noah's comments I understand the issue and likely direction
>> to sort this out now.  Bounced forward to the next CommitFest and back on my
>> plate again.  Guess I'll be learning new and exciting things about
>> translation this week.
>
> I should say that I have more or less finished this one off, since I
> figured you were working on more important things. Just a final round
> of cleanup and commit left, really, so you can take it off your plate
> again if you want to :-)

Finally, to end the bikeshedding, I've applied this patch after
another round of fairly extensive changes.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] Patch to allow users to kill their own queries

2012-01-13 Thread Magnus Hagander
On Fri, Jan 13, 2012 at 14:42, Greg Smith  wrote:
> On 01/03/2012 12:59 PM, Tom Lane wrote:
>>
>> Noah Misch  writes:
>>
>>>
>>> Regarding the other message, avoid composing a translated message from
>>> independently-translated parts.
>>>
>>
>> Yes.  I haven't looked at the patch, but I wonder whether it wouldn't be
>> better to dodge both of these problems by having the subroutine return a
>> success/failure result code, so that the actual ereport can be done at
>> an outer level where the full message (and hint) can be written
>> directly.
>>
>
>
> Between your and Noah's comments I understand the issue and likely direction
> to sort this out now.  Bounced forward to the next CommitFest and back on my
> plate again.  Guess I'll be learning new and exciting things about
> translation this week.

I should say that I have more or less finished this one off, since I
figured you were working on more important things. Just a final round
of cleanup and commit left, really, so you can take it off your plate
again if you want to :-)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] Patch to allow users to kill their own queries

2012-01-13 Thread Greg Smith

On 01/03/2012 12:59 PM, Tom Lane wrote:

Noah Misch  writes:
   

Regarding the other message, avoid composing a translated message from
independently-translated parts.
 

Yes.  I haven't looked at the patch, but I wonder whether it wouldn't be
better to dodge both of these problems by having the subroutine return a
success/failure result code, so that the actual ereport can be done at
an outer level where the full message (and hint) can be written
directly.
   


Between your and Noah's comments I understand the issue and likely 
direction to sort this out now.  Bounced forward to the next CommitFest 
and back on my plate again.  Guess I'll be learning new and exciting 
things about translation this week.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com


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


Re: [HACKERS] Patch to allow users to kill their own queries

2012-01-03 Thread Tom Lane
Noah Misch  writes:
> Regarding the other message, avoid composing a translated message from
> independently-translated parts.

Yes.  I haven't looked at the patch, but I wonder whether it wouldn't be
better to dodge both of these problems by having the subroutine return a
success/failure result code, so that the actual ereport can be done at
an outer level where the full message (and hint) can be written
directly.

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


Re: [HACKERS] Patch to allow users to kill their own queries

2012-01-03 Thread Noah Misch
On Tue, Dec 20, 2011 at 02:30:08PM +0100, Magnus Hagander wrote:
> That said - can someone who knows the translation stuff better than me
> comment on if this is actually going to be translatable, or if it
> violates too many translation rules?

> +pg_signal_backend(int pid, int sig, bool allow_same_role, const char 
> *actionstr, const char *hint)

> + ereport(ERROR,
> + 
> (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +  errmsg("must be superuser to %s other 
> server processes", actionstr),
> +  errhint("%s", hint)));

> + PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM, false,
> +  
> gettext_noop("terminate"),
> +  
> gettext_noop("You can cancel your own processes with pg_cancel_backend().")));
>  }

You need "errhint("%s", _(hint))" or "errhint(hint)" to substitute the
translation at runtime; only the printf-pattern string gets an automatic
message catalog lookup.

Regarding the other message, avoid composing a translated message from
independently-translated parts.  The translator sees this:


#: utils/adt/misc.c:110
#, c-format
msgid "must be superuser to %s other server processes"
msgstr ""

#: utils/adt/misc.c:166
msgid "terminate"
msgstr ""

#: utils/adt/misc.c:167
msgid "You can cancel your own processes with pg_cancel_backend()."
msgstr ""


He'll probably need to read the code to see how the strings go together.  If
we add an alternative to "terminate", not all languages will necessarily have
a translation of the outer message that works for both inner fragments.

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-20 Thread Magnus Hagander
On Mon, Dec 19, 2011 at 15:50, Greg Smith  wrote:
> On 12/18/2011 07:31 AM, Magnus Hagander wrote:
>>
>> * I restructured the if statements, because I had a hard time
>> following the comments around that ;) I find this one easier - but I'm
>> happy to change back if you think your version was more readable.
>
>
> That looks fine.  I highlighted this because I had a feeling there was still
> some gain to be had here, just didn't see it myself.  This works.
>
>
>> * The error message in pg_signal_backend breaks the abstraction,
>> because it specifically talks about terminating the other backend -
>> when it's not supposed to know about that in that function. I think we
>> either need to get rid of the hint completely, or we need to find a
>> way to issue it from the caller. Or pass it as a parameter. It's fine
>> for now since we only have two signals, but we might have more in the
>> future..
>
>
> I feel that including a hint in the pg_terminate_backend case is a UI
> requirement.  If someone has made it as far as discovering that function
> exists, tries calling it, and it fails, the friendly thing to do is point
> them toward a direction that might work better.  Little things like that
> make a huge difference in how friendly the software appears to its users;
> this is even more true in cases where version improvements actually expand
> what can and cannot be done.
>
> My quick and dirty side thinks that just documenting the potential future
> issues would be enough:
>
> "Due to the limited number of callers of this function, the hint message
> here can be certain that pg_terminate_backend provides the only path to
> reach this point.  If more callers to pg_signal_backend appear, a more
> generic hint mechanism might be needed here."
>
> If you must have this more generic mechanism available, I would accept
> re-factoring to provide it instead.  What I wouldn't want to live with is a
> commit of this where the hint goes away completely.  It's taken a long time
> chopping the specification to get this feature sorted out; we might as well
> make what's left be the best it can be now.

How about something like this - passing it in as a parameter?

That said - can someone who knows the translation stuff better than me
comment on if this is actually going to be translatable, or if it
violates too many translation rules?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14244,8 +14244,8 @@ SELECT set_config('log_statement_stats', 'off', false);

 The functions shown in  send control signals to
-other server processes.  Use of these functions is restricted
-to superusers.
+other server processes.  Use of these functions is usually restricted
+to superusers, with noted exceptions.

 

@@ -14262,7 +14262,10 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_cancel_backend(pid int)
 
boolean
-   Cancel a backend's current query
+   Cancel a backend's current query.  You can execute this against
+another backend that has exactly the same role as the user calling the
+function.  In all other cases, you must be a superuser.
+
   
   

@@ -14304,6 +14307,10 @@ SELECT set_config('log_statement_stats', 'off', false);
 postgres processes on the server (using
 ps on Unix or the Task
 Manager on Windows).
+For the less restrictive pg_cancel_backend, the role of an
+active backend can be found from
+the usename column of the
+pg_stat_activity view.

 

diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..45520b6 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -30,6 +30,7 @@
 #include "postmaster/syslogger.h"
 #include "storage/fd.h"
 #include "storage/pmsignal.h"
+#include "storage/proc.h"
 #include "storage/procarray.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
@@ -70,15 +71,45 @@ current_query(PG_FUNCTION_ARGS)
 }
 
 /*
- * Functions to send signals to other backends.
+ * Send a signal to another backend.
+ * If allow_same_role is false, actionstr must be set to a string
+ * indicating what the signal does, to be inserted in the error message, and
+ * hint should be set to a hint to be sent along with this message.
  */
 static bool
-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, bool allow_same_role, const char *actionstr, const char *hint)
 {
+	PGPROC	   *proc;
+
 	if (!superuser())
-		ereport(ERROR,
-(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-			(errmsg("must be superuser to signal other server processes";
+	{
+		if (allow_same_role)
+		{
+			/*
+			 * When same role permission is allowed, check for matching roles.
+			 * Trust that BackendPidGetProc will return N

Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-19 Thread Greg Smith

On 12/18/2011 07:31 AM, Magnus Hagander wrote:

* I restructured the if statements, because I had a hard time
following the comments around that ;) I find this one easier - but I'm
happy to change back if you think your version was more readable.


That looks fine.  I highlighted this because I had a feeling there was 
still some gain to be had here, just didn't see it myself.  This works.



* The error message in pg_signal_backend breaks the abstraction,
because it specifically talks about terminating the other backend -
when it's not supposed to know about that in that function. I think we
either need to get rid of the hint completely, or we need to find a
way to issue it from the caller. Or pass it as a parameter. It's fine
for now since we only have two signals, but we might have more in the
future..


I feel that including a hint in the pg_terminate_backend case is a UI 
requirement.  If someone has made it as far as discovering that function 
exists, tries calling it, and it fails, the friendly thing to do is 
point them toward a direction that might work better.  Little things 
like that make a huge difference in how friendly the software appears to 
its users; this is even more true in cases where version improvements 
actually expand what can and cannot be done.


My quick and dirty side thinks that just documenting the potential 
future issues would be enough:


"Due to the limited number of callers of this function, the hint message 
here can be certain that pg_terminate_backend provides the only path to 
reach this point.  If more callers to pg_signal_backend appear, a more 
generic hint mechanism might be needed here."


If you must have this more generic mechanism available, I would accept 
re-factoring to provide it instead.  What I wouldn't want to live with 
is a commit of this where the hint goes away completely.  It's taken a 
long time chopping the specification to get this feature sorted out; we 
might as well make what's left be the best it can be now.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-18 Thread Robert Haas
On Sat, Dec 17, 2011 at 11:58 PM, Tom Lane  wrote:
> I think this argument is bogus: if this is a real issue, then no use of
> kill() anytime, by anyone, is safe.  In practice I believe that Unix
> systems avoid recycling PIDs right away so as to offer some protection.

I'm not sure they do anything more sophisticated than cycling through
a sufficiently-large PID space, but whether it's that or something
else, I guess it must be adequate or they'd have enlarged the space...

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-18 Thread Magnus Hagander
On Fri, Dec 16, 2011 at 13:31, Greg Smith  wrote:
> On 12/14/2011 05:24 AM, Magnus Hagander wrote:
>>
>> How about passing a parameter to pg_signal_backend? Making
>> pg_signal_backend(int pid, int sig, bool allow_samerole)?
>>
>
>
> That works, got rid of the parts I didn't like and allowed some useful minor
> restructuring.  I also made the HINT better and match style guidelines:
>
> gsmith=> select pg_terminate_backend(21205);
>
> ERROR:  must be superuser to terminate other server processes
> HINT:  You can cancel your own processes with pg_cancel_backend().
> gsmith=> select pg_cancel_backend(21205);
>  pg_cancel_backend
> ---
>  t
>
> New rev attached and pushed to
> https://github.com/greg2ndQuadrant/postgres/tree/cancel-backend (which is
> *not* the same branch as I used last time; don't ask why, long story)
>
> I considered some additional ways to restructure the checks that could
> remove a further line or two from the logic here, but they all made the
> result seem less readable to me.  And this is not a high performance code
> path.  I may have gone a bit too far with the comment additions though, so
> feel free to trim that back.  It kept feeling weird to me that none of the
> individual signaling functions had their own intro comments.  I added all
> those.
>
> I also wrote up a commentary on the PID wraparound race condition
> possibility Josh brought up.  Some research shows that pid assignment on
> some systems is made more secure by assigning new ones randomly.  That seems
> like it would make it possible to have a pid get reused much faster than on
> the usual sort of system that does sequential assignment and wraparound.  A
> reuse collision still seems extremely unlikely though.  With the new
> comments, at least a future someone who speculates on this will know how
> much thinking went into the current implementation:  enough to notice, not
> enough to see anything worth doing about it.  Maybe that's just wasted lines
> of text?
>
> With so little grief on the last round, I'm going to guess this one will
> just get picked up by Magnus to commit next.  Marking accordingly and moved
> to the current CommitFest.

I was going to, but I noticed a few things:

* I restructured the if statements, because I had a hard time
following the comments around that ;) I find this one easier - but I'm
happy to change back if you think your version was more readable.

* The error message in pg_signal_backend breaks the abstraction,
because it specifically talks about terminating the other backend -
when it's not supposed to know about that in that function. I think we
either need to get rid of the hint completely, or we need to find a
way to issue it from the caller. Or pass it as a parameter. It's fine
for now since we only have two signals, but we might have more in the
future..

* I gave it a run of pgindent ;)


In the attached updated patch I've just removed the HINT and changed
the reference from"terminate" to "signal". But I'd like your input
onthat before I commit :-)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14244,8 +14244,8 @@ SELECT set_config('log_statement_stats', 'off', false);

 The functions shown in  send control signals to
-other server processes.  Use of these functions is restricted
-to superusers.
+other server processes.  Use of these functions is usually restricted
+to superusers, with noted exceptions.

 

@@ -14262,7 +14262,10 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_cancel_backend(pid int)
 
boolean
-   Cancel a backend's current query
+   Cancel a backend's current query.  You can execute this against
+another backend that has exactly the same role as the user calling the
+function.  In all other cases, you must be a superuser.
+
   
   

@@ -14304,6 +14307,10 @@ SELECT set_config('log_statement_stats', 'off', false);
 postgres processes on the server (using
 ps on Unix or the Task
 Manager on Windows).
+For the less restrictive pg_cancel_backend, the role of an
+active backend can be found from
+the usename column of the
+pg_stat_activity view.

 

diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..d7f2435 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -30,6 +30,7 @@
 #include "postmaster/syslogger.h"
 #include "storage/fd.h"
 #include "storage/pmsignal.h"
+#include "storage/proc.h"
 #include "storage/procarray.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
@@ -70,15 +71,41 @@ current_query(PG_FUNCTION_ARGS)
 }
 
 /*
- * Functions to send signals to other backends.
+ * Send a signal to another backend
  */
 static bool
-p

Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-17 Thread Tom Lane
Robert Haas  writes:
> On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith  wrote:
>> ... If you assume someone can run through all the
>> PIDs between those checks and the kill, the system is already broken that
>> way.

> From a theoretical point of view, I believe it to be slightly
> different.  If a superuser sends a kill, they will certainly be
> authorized to kill whatever they end up killing, because they are
> authorized to kill anything.  On the other hand, the proposed patch
> would potentially result - in the extremely unlikely event of a
> super-fast PID wraparound - in someone cancelling a query they
> otherwise wouldn't have been able to cancel.

> In practice, the chances of this seem fairly remote.

I think this argument is bogus: if this is a real issue, then no use of
kill() anytime, by anyone, is safe.  In practice I believe that Unix
systems avoid recycling PIDs right away so as to offer some protection.

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Alvaro Herrera

Excerpts from Greg Smith's message of vie dic 16 11:19:52 -0300 2011:
> On 12/16/2011 08:42 AM, Robert Haas wrote:
> > the proposed patch would potentially result - in the extremely 
> > unlikely event of a
> > super-fast PID wraparound - in someone cancelling a query they
> > otherwise wouldn't have been able to cancel.
> >
> 
> So how might this get exploited?
> 
> -Attach a debugger and put a breakpoint between the check and the kill

If you can attach a debugger to a backend, you already have the
credentials to the user running postmaster, which we assume to be a
"game over" condition.

I guess a more interesting exploitation would be to do this until the
killing backend randomly wins the race condition against a dying backend
and a new one starting up; this would require very rapid reuse of the
PID, plus very rapid startup of the new proces.  I think we already
assume that this is not the case in other code paths, even on systems
that randomly (instead of sequentially) assign PIDs.

-- 
Álvaro Herrera 
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Magnus Hagander
On Friday, December 16, 2011, Greg Smith wrote:

> On 12/16/2011 08:42 AM, Robert Haas wrote:
>
>> the proposed patch would potentially result - in the extremely unlikely
>> event of a
>> super-fast PID wraparound - in someone cancelling a query they
>> otherwise wouldn't have been able to cancel.
>>
>>
>
> So how might this get exploited?
>
> -Attach a debugger and put a breakpoint between the check and the kill
>

Once you've attached a debugger, you've already won. You can inject
arbitrary instructions at this point, no?


> -Fork processes to get close to your target
> -Wait for a process you want to mess with to appear at the PID you're
> waiting for.  If you miss it, repeat fork bomb and try again.
> -Resume the debugger to kill the other user's process
>
> If I had enough access to launch this sort of attack, I think I'd find
> mayhem elsewhere more a more profitable effort.  Crazy queries, work_mem
> abuse, massive temp file generation, trying to get the OOM killer involved;
> seems like there's bigger holes than this already.
>

"killall -9 postgres" is even easier.


//Magnus


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Magnus Hagander
On Friday, December 16, 2011, Robert Haas wrote:

> On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith 
> >
> wrote:
> > This is a problem with the existing code though, and the proposed changes
> > don't materially alter that; there's just another quick check in one path
> > through.  Right now we check if someone is superuser, then if it's a
> backend
> > PID, then we send the signal.  If you assume someone can run through all
> the
> > PIDs between those checks and the kill, the system is already broken that
> > way.
>
> From a theoretical point of view, I believe it to be slightly
> different.  If a superuser sends a kill, they will certainly be
> authorized to kill whatever they end up killing, because they are
> authorized to kill anything.  On the other hand, the proposed patch
>

Not necessarily. What if it's recycled as a backend in a different postgres
installation. Or just a cronjob or shell running as the same user?

Sure, you can argue that the superuser can destroy anything he wants - but
in that case, why do we have a check for this at all in the first place?

I think we can safely say that any OS that actually manages to recycle the
PID in the short time it takes to get between those instructions is so
broken we don't need to care about that.


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Greg Smith

On 12/16/2011 08:42 AM, Robert Haas wrote:
the proposed patch would potentially result - in the extremely 
unlikely event of a

super-fast PID wraparound - in someone cancelling a query they
otherwise wouldn't have been able to cancel.
   


So how might this get exploited?

-Attach a debugger and put a breakpoint between the check and the kill
-Fork processes to get close to your target
-Wait for a process you want to mess with to appear at the PID you're 
waiting for.  If you miss it, repeat fork bomb and try again.

-Resume the debugger to kill the other user's process

If I had enough access to launch this sort of attack, I think I'd find 
mayhem elsewhere more a more profitable effort.  Crazy queries, work_mem 
abuse, massive temp file generation, trying to get the OOM killer 
involved; seems like there's bigger holes than this already.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Robert Haas
On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith  wrote:
> This is a problem with the existing code though, and the proposed changes
> don't materially alter that; there's just another quick check in one path
> through.  Right now we check if someone is superuser, then if it's a backend
> PID, then we send the signal.  If you assume someone can run through all the
> PIDs between those checks and the kill, the system is already broken that
> way.

From a theoretical point of view, I believe it to be slightly
different.  If a superuser sends a kill, they will certainly be
authorized to kill whatever they end up killing, because they are
authorized to kill anything.  On the other hand, the proposed patch
would potentially result - in the extremely unlikely event of a
super-fast PID wraparound - in someone cancelling a query they
otherwise wouldn't have been able to cancel.

In practice, the chances of this seem fairly remote.

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-16 Thread Greg Smith

On 12/14/2011 05:24 AM, Magnus Hagander wrote:

How about passing a parameter to pg_signal_backend? Making
pg_signal_backend(int pid, int sig, bool allow_samerole)?
   


That works, got rid of the parts I didn't like and allowed some useful 
minor restructuring.  I also made the HINT better and match style 
guidelines:


gsmith=> select pg_terminate_backend(21205);
ERROR:  must be superuser to terminate other server processes
HINT:  You can cancel your own processes with pg_cancel_backend().
gsmith=> select pg_cancel_backend(21205);
 pg_cancel_backend
---
 t

New rev attached and pushed to 
https://github.com/greg2ndQuadrant/postgres/tree/cancel-backend (which 
is *not* the same branch as I used last time; don't ask why, long story)


I considered some additional ways to restructure the checks that could 
remove a further line or two from the logic here, but they all made the 
result seem less readable to me.  And this is not a high performance 
code path.  I may have gone a bit too far with the comment additions 
though, so feel free to trim that back.  It kept feeling weird to me 
that none of the individual signaling functions had their own intro 
comments.  I added all those.


I also wrote up a commentary on the PID wraparound race condition 
possibility Josh brought up.  Some research shows that pid assignment on 
some systems is made more secure by assigning new ones randomly.  That 
seems like it would make it possible to have a pid get reused much 
faster than on the usual sort of system that does sequential assignment 
and wraparound.  A reuse collision still seems extremely unlikely 
though.  With the new comments, at least a future someone who speculates 
on this will know how much thinking went into the current 
implementation:  enough to notice, not enough to see anything worth 
doing about it.  Maybe that's just wasted lines of text?


With so little grief on the last round, I'm going to guess this one will 
just get picked up by Magnus to commit next.  Marking accordingly and 
moved to the current CommitFest.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT set_config('log_statement_stats',
*** 14244,14251 
 
  The functions shown in  send control signals to
! other server processes.  Use of these functions is restricted
! to superusers.
 
  
 
--- 14244,14251 
 
  The functions shown in  send control signals to
! other server processes.  Use of these functions is usually restricted
! to superusers, with noted exceptions.
 
  
 
*** SELECT set_config('log_statement_stats',
*** 14262,14268 
  pg_cancel_backend(pid int)
  
 boolean
!Cancel a backend's current query


 
--- 14262,14271 
  pg_cancel_backend(pid int)
  
 boolean
!Cancel a backend's current query.  You can execute this against
! another backend that has exactly the same role as the user calling the
! function.  In all other cases, you must be a superuser.
! 


 
*** SELECT set_config('log_statement_stats',
*** 14304,14309 
--- 14307,14316 
  postgres processes on the server (using
  ps on Unix or the Task
  Manager on Windows).
+ For the less restrictive pg_cancel_backend, the role of an
+ active backend can be found from
+ the usename column of the
+ pg_stat_activity view.
 
  
 
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..1b7b75b 100644
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 30,35 
--- 30,36 
  #include "postmaster/syslogger.h"
  #include "storage/fd.h"
  #include "storage/pmsignal.h"
+ #include "storage/proc.h"
  #include "storage/procarray.h"
  #include "tcop/tcopprot.h"
  #include "utils/builtins.h"
*** current_query(PG_FUNCTION_ARGS)
*** 70,84 
  }
  
  /*
!  * Functions to send signals to other backends.
   */
  static bool
! pg_signal_backend(int pid, int sig)
  {
! 	if (!superuser())
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 			(errmsg("must be superuser to signal other server processes";
  
  	if (!IsBackendPid(pid))
  	{
--- 71,113 
  }
  
  /*
!  * Send a signal to another backend
   */
  static bool
! pg_signal_backend(int pid, int sig, bool allow_same_role)
  {
! 	PGPROC	*proc;
! 	bool	allowed = superuser();
! 
! 	if (!allowed && allow_same_role)
! 	{
! 		/*
! 		 * When same role permission is allowed, check for matching roles.  Trust
! 		 * that BackendPidGetProc will return NULL if the pid isn't valid, even
! 		 * though t

Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-15 Thread Greg Smith

On 12/15/2011 07:36 PM, Josh Kupershmidt wrote:

The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend().


This is a problem with the existing code though, and the proposed 
changes don't materially alter that; there's just another quick check in 
one path through.  Right now we check if someone is superuser, then if 
it's a backend PID, then we send the signal.  If you assume someone can 
run through all the PIDs between those checks and the kill, the system 
is already broken that way.



I notice that BackendPidGetProc() has the comment:

   ...  Note that it is up to the caller to be
   sure that the question remains meaningful for long enough for the
   answer to be used ...

So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend().


Right, the the possibility you're raising is the obvious flaw with this 
approach.  Currently the only consumers of BackendPidGetProc or 
IsBackendPid are these cancel/terminate functions.  As you measured, 
running through the PIDs fast enough to outrace any of that code is 
unlikely.  I'm sure a lot of other UNIX apps would break, too, if that 
ever became untrue.  The sort of thing that comment is warning is that 
you wouldn't want to do something like grab a PID, vacuum a table, and 
then assume that PID was still valid.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-15 Thread Josh Kupershmidt
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith  wrote:
> Same-user cancels, but not termination.  Only this, and nothing more.

+1 from me on this approach. I think enough people have clamored for
this simple approach which solves the common-case.

> There's one obvious and questionable design decision I made to highlight.
>  Right now the only consumers of pg_signal_backend are the cancel and
> terminate calls.  What I did was make pg_signal_backend more permissive,
> adding the idea that role equivalence = allowed, and therefore granting that
> to anything else that might call it.  And then I put a stricter check on
> termination.  This results in a redundant check of superuser on the
> termination check, and the potential for mis-using pg_signal_backend. . .

I think that's OK, it should be easy enough to reorganize if more
callers to pg_signal_backend() happen to come along.

The only niggling concern I have about this patch (and, I think, all
similar ones proposed) is the possible race condition between the
permissions checking and the actual call of kill() inside
pg_signal_backend(). I fooled around with this by using gdb to attach
to Backend #1, stuck a breakpoint right before the call to:

if (kill(-pid, sig))

and ran a pg_cancel_backend() of a same-role Backend #2. Then, with
the permissions checking passed, I exited Backend #2 and used a script
to call fork() in a loop until my system PIDs had wrapped around to a
few PIDs before the one Backend #2 had held. Then, I opened a few
superuser connections until I had one with the same PID which Backend
#2 previously had.

After I released the breakpoint in gdb on Backend #1, it happily sent
a SIGINT to my superuser backend.

I notice that BackendPidGetProc() has the comment:

  ...  Note that it is up to the caller to be
  sure that the question remains meaningful for long enough for the
  answer to be used ...

So someone has mulled this over before. It took my script maybe 15-20
minutes to cause a wraparound by running fork() in a loop, and that
wraparound would somehow have to occur within the short execution of
pg_signal_backend(). I'm not super worried about the patch from a
security perspective, just thought I'd point this out.

Josh

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-14 Thread Tom Lane
Magnus Hagander  writes:
> On Tue, Dec 13, 2011 at 11:59, Greg Smith  wrote:
>> HINT:  you can use pg_cancel_backend() on your own processes

> That HINT sounds a bit weird to me. "you can cancel your own queries
> using pg_cancel_backend() instead" or something like that?

Doesn't follow message style guidelines either ;-).  Hints should be
complete sentences, with initial cap and ending period.
http://www.postgresql.org/docs/devel/static/error-style-guide.html

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-14 Thread Greg Smith

On 12/14/2011 05:24 AM, Magnus Hagander wrote:

How about passing a parameter to pg_signal_backend? Making
pg_signal_backend(int pid, int sig, bool allow_samerole)?
   


That sounds like it will result in less code, and make the API I was 
documenting be obvious from the parameters instead.  I'll refactor to do 
that, tweak the hint message, and resubmit.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-14 Thread Magnus Hagander
On Tue, Dec 13, 2011 at 11:59, Greg Smith  wrote:
> The submission from Edward Muller I'm replying to is quite similar to what
> the other raging discussion here decided was the right level to target.
>  There was one last year from Josh Kupershmidt with similar goals:
>  http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php  A good
> place to start is the concise summary of the new specification goal that Tom
> made in the other thread:
>
>> If allowing same-user cancels is enough to solve 95% or 99% of the
>> real-world use cases, let's just do that.
>
> Same-user cancels, but not termination.  Only this, and nothing more.

Good.

> Appropriate credits here would go Josh Kupershmidt, Edward Muller, and then
> myself; everyone did an equally useful chunk of this in that order.  It's
> all packaged up for useful gitsumption at
> https://github.com/greg2ndQuadrant/postgres/tree/cancel_backend too.  I
> attached it to the next CommitFest:
>  https://commitfest.postgresql.org/action/patch_view?id=722 but would enjoy
> seeing a stake finally put through its evil heart before then, as I don't
> think there's much left to do now.
>
> test=> select pg_terminate_backend(28154);
> ERROR:  must be superuser to terminate other server processes
> HINT:  you can use pg_cancel_backend() on your own processes

That HINT sounds a bit weird to me. "you can cancel your own queries
using pg_cancel_backend() instead" or something like that?


> Victory over the evil sleeping backend is complete, without a superuser in
> sight.

\o/


> There's one obvious and questionable design decision I made to highlight.
>  Right now the only consumers of pg_signal_backend are the cancel and
> terminate calls.  What I did was make pg_signal_backend more permissive,
> adding the idea that role equivalence = allowed, and therefore granting that
> to anything else that might call it.  And then I put a stricter check on
> termination.  This results in a redundant check of superuser on the
> termination check, and the potential for mis-using pg_signal_backend.  I
> documented all that and liked the result; it feels better to me to have
> pg_signal_backend provide an API that is more flexible here.  Pushback to
> structure this differently is certainly possible though, and I'm happy to
> iterate the patch to address that.  It might drift back toward something
> closer to Josh's original design.

How about passing a parameter to pg_signal_backend? Making
pg_signal_backend(int pid, int sig, bool allow_samerole)?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-13 Thread Greg Smith
The submission from Edward Muller I'm replying to is quite similar to 
what the other raging discussion here decided was the right level to 
target.  There was one last year from Josh Kupershmidt with similar 
goals:  http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php  
A good place to start is the concise summary of the new specification 
goal that Tom made in the other thread:


> If allowing same-user cancels is enough to solve 95% or 99% of the 
real-world use cases, let's just do that.


Same-user cancels, but not termination.  Only this, and nothing more.

Relative to that goal, Ed's patch was too permissive for termination, 
and since he's new to this code it didn't check all the error conditions 
possible here.  Josh's patch had many of the right error checks, but it 
was more code than I liked for his slightly different permissions 
change.  And its attempts to be helpful leaked role information.  (That 
may have been just debugging debris left for review purposes)  I mashed 
the best bits of both together, tried to simplify the result, then 
commented heavily upon the race conditions and design decisions the code 
reflects.  Far as I can tell the patch is feature complete, including 
documentation.


Appropriate credits here would go Josh Kupershmidt, Edward Muller, and 
then myself; everyone did an equally useful chunk of this in that 
order.  It's all packaged up for useful gitsumption at 
https://github.com/greg2ndQuadrant/postgres/tree/cancel_backend too.  I 
attached it to the next CommitFest:  
https://commitfest.postgresql.org/action/patch_view?id=722 but would 
enjoy seeing a stake finally put through its evil heart before then, as 
I don't think there's much left to do now.


To demo I start with a limited user and a crazy, must be stopped backend:

$ createuser test
Shall the new role be a superuser? (y/n) n
Shall the new role be allowed to create databases? (y/n) n
Shall the new role be allowed to create more new roles? (y/n) n
$ psql -U test
test=> select pg_sleep(100);

Begin another session, find and try to terminate; get rejected with a 
hint, then follow it to cancel:


test=> select procpid,usename,current_query from pg_stat_activity;
-[ RECORD 1 ]-+
procpid   | 28154
usename   | test
current_query | select pg_sleep(100);

test=> select pg_terminate_backend(28154);
ERROR:  must be superuser to terminate other server processes
HINT:  you can use pg_cancel_backend() on your own processes
test=> select pg_cancel_backend(28154);
-[ RECORD 1 ]-+--
pg_cancel_backend | t

And then this is shown on the first one:

test=> select pg_sleep(100);
ERROR:  canceling statement due to user request

Victory over the evil sleeping backend is complete, without a superuser 
in sight.


There's one obvious and questionable design decision I made to 
highlight.  Right now the only consumers of pg_signal_backend are the 
cancel and terminate calls.  What I did was make pg_signal_backend more 
permissive, adding the idea that role equivalence = allowed, and 
therefore granting that to anything else that might call it.  And then I 
put a stricter check on termination.  This results in a redundant check 
of superuser on the termination check, and the potential for mis-using 
pg_signal_backend.  I documented all that and liked the result; it feels 
better to me to have pg_signal_backend provide an API that is more 
flexible here.  Pushback to structure this differently is certainly 
possible though, and I'm happy to iterate the patch to address that.  It 
might drift back toward something closer to Josh's original design.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..f145c3f 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT set_config('log_statement_stats',
*** 14244,14251 
 
  The functions shown in  send control signals to
! other server processes.  Use of these functions is restricted
! to superusers.
 
  
 
--- 14244,14251 
 
  The functions shown in  send control signals to
! other server processes.  Use of these functions is usually restricted
! to superusers, with noted exceptions.
 
  
 
*** SELECT set_config('log_statement_stats',
*** 14262,14268 
  pg_cancel_backend(pid int)
  
 boolean
!Cancel a backend's current query


 
--- 14262,14271 
  pg_cancel_backend(pid int)
  
 boolean
!Cancel a backend's current query.  You can execute this against
! another backend that has exactly the same role as the user calling the
! function.  In all other cases, you must be a superuser.
! 


 
***

Re: [HACKERS] Patch to allow users to kill their own queries

2011-12-01 Thread Greg Smith

On 11/16/2011 01:28 PM, Daniel Farina wrote:

As it would turn out, a patch for this has already been submitted:
http://archives.postgresql.org/pgsql-hackers/2011-10/msg1.php

There was some wrangling on whether it needs to be extended to be
useful, but for our purposes the formulation already posted already
captures vital value for us, and in that form appears to be fairly
uncontentious. I have moved it to the current commitfest, with a
comment linking to the 'please revive this patch' thread whereby a
second glance at what to do about this was conducted.


Yeah, that one got a raw deal; it should be in the *current* 
CommitFest--you had it in the next one.  I'll join the chorus to just 
allow people to fire just the pg_cancel_backend pea shooter foot gun 
targeted only at their own feet, make most users happy, and punt 
anything more complicated off as troublesome relative to its benefit.  
That's what Daniel has said, what his co-worker Edward also implemented, 
what Noah thought was good enough given other security mechanisms in 
place, and what Tom thinks is reasonable too.


This I feel is important, so I'm going to add myself as the next 
reviewer and include it in my testing run tomorrow.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-11-16 Thread Daniel Farina
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner
 wrote:
> Edward Muller  wrote:
>
>> Looking for comments ...
>>
>> https://gist.github.com/be937d3a7a5323c73b6e
>>
>> We'd like to get this, or something like it, into 9.2

On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner
 wrote:
> Edward Muller  wrote:
>
>> Looking for comments ...
>>
>> https://gist.github.com/be937d3a7a5323c73b6e
>>
>> We'd like to get this, or something like it, into 9.2

As it would turn out, a patch for this has already been submitted:

http://archives.postgresql.org/pgsql-hackers/2011-10/msg1.php

There was some wrangling on whether it needs to be extended to be
useful, but for our purposes the formulation already posted already
captures vital value for us, and in that form appears to be fairly
uncontentious. I have moved it to the current commitfest, with a
comment linking to the 'please revive this patch' thread whereby a
second glance at what to do about this was conducted.  The link
follows:

https://commitfest.postgresql.org/action/patch_view?id=541

> If you want it to be seriously considered, you should post the patch
> to this list, which makes it part of the permanent archives and
> indicates your willingness to place the code under the PostgreSQL
> license.

Although technical mailing lists are not primarily a place of
reflection and sensitivity, I do think that wording addressed to a new
participant could have been kinder.  Perhaps, "Unfortunately we cannot
accept or even read your patch because of licensing concerns, would
you please follow the following patch submission guidelines?" .

-- 
fdr

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-11-16 Thread Edward Muller
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner
 wrote:
> Edward Muller  wrote:
>
>> Looking for comments ...
>>
>> https://gist.github.com/be937d3a7a5323c73b6e
>>
>> We'd like to get this, or something like it, into 9.2
>
> If you want it to be seriously considered, you should post the patch
> to this list, which makes it part of the permanent archives and
> indicates your willingness to place the code under the PostgreSQL
> license.

inline 

>
> http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission

I think this was done right.

>

[snip]

>
> Any chance you could help review patches in the CF in progress, to
> help get others' time freed up sooner?:
>
> https://commitfest.postgresql.org/action/commitfest_view/inprogress

Well, I'm totally new to the code base, I don't write in *C* very
often (probably obvious from my patch). Maybe over time

>
> It's a good way to become familiar with the process, so that you can
> better move your own patch along.
>
> -Kevin
>


user_signal.patch
Description: Binary data

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


Re: [HACKERS] Patch to allow users to kill their own queries

2011-11-16 Thread Kevin Grittner
Edward Muller  wrote:
 
> Looking for comments ...
> 
> https://gist.github.com/be937d3a7a5323c73b6e
> 
> We'd like to get this, or something like it, into 9.2
 
If you want it to be seriously considered, you should post the patch
to this list, which makes it part of the permanent archives and
indicates your willingness to place the code under the PostgreSQL
license.
 
http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission
 
Unfortunately, you're a day late in terms of getting it reviewed in
the just-started CommitFest, but another will start in two months.
 
https://commitfest.postgresql.org/action/commitfest_view/open
 
Of course, you're free to talk about it until then, and perhaps
someone will take a look at it before the next CF; but a lot of
attention will be focused on the patches submitted by the start of
the current CF.
 
Any chance you could help review patches in the CF in progress, to
help get others' time freed up sooner?:
 
https://commitfest.postgresql.org/action/commitfest_view/inprogress
 
It's a good way to become familiar with the process, so that you can
better move your own patch along.
 
-Kevin

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


[HACKERS] Patch to allow users to kill their own queries

2011-11-16 Thread Edward Muller
Looking for comments ...

https://gist.github.com/be937d3a7a5323c73b6e

We'd like to get this, or something like it, into 9.2

PS Subscribing to psql-hack...@postgresql.org just spins for me.

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