Re: [PATCH 2.6.7] kill(2), killpg(2) wrongly fail with EPERM

Re: [PATCH 2.6.7] kill(2), killpg(2) wrongly fail with EPERM

[Linus Torvalds]

On Wed, 16 Jun 2004, Michael Kerrisk wrote:
> 
> The following patch for 2.6.7 fixes the problem.  Please apply.

How about this imho nicer version instead? It results in the main loop
being just:

        success = 0;
        retval = -ESRCH;
        for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
                int err = group_send_sig_info(sig, info, p);
                success |= !err;
                retval = err;  
        }
        return success ? 0 : retval;

which seems sensible. If _any_ group-send succeeded, we want to return 
success (ie this is not a EPERM vs everything else issue).

Does this work for you?

		Linus

-----
===== kernel/signal.c 1.120 vs edited =====
--- 1.120/kernel/signal.c	Wed Jun  9 01:46:51 2004
+++ edited/kernel/signal.c	Wed Jun 16 09:09:51 2004
@@ -1071,23 +1071,19 @@
 	struct task_struct *p;
 	struct list_head *l;
 	struct pid *pid;
-	int retval;
-	int found;
+	int retval, success;
 
 	if (pgrp <= 0)
 		return -EINVAL;
 
-	found = 0;
-	retval = 0;
+	success = 0;
+	retval = -ESRCH;
 	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
-		int err;
-
-		found = 1;
-		err = group_send_sig_info(sig, info, p);
-		if (!retval)
-			retval = err;
+		int err = group_send_sig_info(sig, info, p);
+		success |= !err;
+		retval = err;
 	}
-	return found ? retval : -ESRCH;
+	return success ? 0 : retval;
 }
 
 int

Re: [PATCH 2.6.7] kill(2), killpg(2) wrongly fail with EPERM

[Michael Kerrisk]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="us-ascii", Size: 2290 bytes --]

Gidday Linus,

> On Wed, 16 Jun 2004, Michael Kerrisk wrote:
> > 
> > The following patch for 2.6.7 fixes the problem.  Please apply.
> 
> How about this imho nicer version instead? It results in the main loop
> being just:
> 
>         success = 0;
>         retval = -ESRCH;
>         for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
>                 int err = group_send_sig_info(sig, info, p);
>                 success |= !err;
>                 retval = err;  
>         }
>         return success ? 0 : retval;

Yes, it is nicer.

> which seems sensible. If _any_ group-send succeeded, we want to return 
> success (ie this is not a EPERM vs everything else issue).
> 
> Does this work for you?

Well, in terms of SUSv3/POSIX, I don't think there's a problem, 
since the only errors trhat are specified for kill()/killpg() 
are EPERM, ESRCH, and EINVAL (invalid signal number).  Aside 
from the fact that I didn't spot the nice way, I wrote my 
patch as I did since I was worried about the possibility of 
some other Linux-specific errno values creeping around in the 
woodwork.  But a little further investigation seems to show that
there aren't other cases to worry about (EAGAIN in send_sig()
doesn't apply for kill()/killpg().  So, your patch is better, 
since simpler.  I've tested it, and it works as I would expect 
for EPERM.

Thanks,

Michael


> -----
> ===== kernel/signal.c 1.120 vs edited =====
> --- 1.120/kernel/signal.c	Wed Jun  9 01:46:51 2004
> +++ edited/kernel/signal.c	Wed Jun 16 09:09:51 2004
> @@ -1071,23 +1071,19 @@
>  	struct task_struct *p;
>  	struct list_head *l;
>  	struct pid *pid;
> -	int retval;
> -	int found;
> +	int retval, success;
>  
>  	if (pgrp <= 0)
>  		return -EINVAL;
>  
> -	found = 0;
> -	retval = 0;
> +	success = 0;
> +	retval = -ESRCH;
>  	for_each_task_pid(pgrp, PIDTYPE_PGID, p, l, pid) {
> -		int err;
> -
> -		found = 1;
> -		err = group_send_sig_info(sig, info, p);
> -		if (!retval)
> -			retval = err;
> +		int err = group_send_sig_info(sig, info, p);
> +		success |= !err;
> +		retval = err;
>  	}
> -	return found ? retval : -ESRCH;
> +	return success ? 0 : retval;
>  }
>  
>  int
> 

-- 
+++ Jetzt WLAN-Router für alle DSL-Einsteiger und Wechsler +++
GMX DSL-Powertarife zudem 3 Monate gratis* http://www.gmx.net/dsl