rsync - Mar 2002 - [PATCH] rsync kills all user processes on fork failure

I filed this as bug 4150 http://rsync.samba.org/cgi-bin/rsync/

We had a few hundred hung processes and reached the limit for "max user
processes" (see the ulimit command).   rsync did not handle this very
well.  It passed -1 as a PID to kill().  When kill() gets -1, it kills all
of the processes owned by that user, or if run by root, all processes.

I attached a one line fix.  Let me know if I used the wrong format for a
patch.  If the result of fork is -1 don't put it in the list of pids to
kill.

Reviewing the mailing list, I saw that this happened to at least one other
person, see http://lists.samba.org/pipermail/rsync/2001-April/004158.html
Back in April of 2001, Randy Kramer wrote:
> The rsync server often quit for me when I used the -c option,
> generally because something on the server end decided to kill rsync.
I duplicated the problem with version 2.5.4 on a Linux system.  I used
another user ID, "notme", so that when rsync killed all the processes,
it
wouldn't kill my shell.  "ulimit -u 5" sets the max number of user
processes to 5.  I used the sleep command as my victim processes, it's
annoying to have something usefull killed when duplicating a bug.

bash$ su notme
Password:
bash$ ulimit -u 5
bash$ sleep 100 & sleep 100 & sleep 100 &
[1] 16234
[2] 16235
[3] 16236
bash$ # There are now 4 processes running as "notme", bash and 3
sleeps.
bash$ whoami
notme
bash$ ./rsync -e /usr/bin/rsh -a ~/a bandit:b
fork: Resource temporarily unavailable
rsync error: error in IPC code (code 14) at util.c(137)
bash$ whoami
me
bash$ # all of the notme processes were killed.


None of this has anything to do with the bug in our scripts that lead to
hundreds of hung processes.


==================================================================RCS file:
RCS/util.c,v
retrieving revision 1.1
diff -u -r1.1 util.c
--- util.c      2002/03/19 15:01:28     1.1
+++ util.c      2002/03/19 15:29:32
@@ -483,7 +483,7 @@
 {
        pid_t newpid = fork();

-       if (newpid) {
+       if (newpid > 0) {
                all_pids[num_pids++] = newpid;
        }
        return newpid;

--
Paul Haas, paulh@hamjudo.com http://hamjudo.com

On 19 Mar 2002, Paul Haas <paulh@hamjudo.com>
wrote:
> I filed this as bug 4150 http://rsync.samba.org/cgi-bin/rsync/
> 
> We had a few hundred hung processes and reached the limit for "max
user
> processes" (see the ulimit command).   rsync did not handle this very
> well.  It passed -1 as a PID to kill().  When kill() gets -1, it kills all
> of the processes owned by that user, or if run by root, all processes.
 
Bloody hell.

Yes, I think that's a real bug.  Thanks for yourhelp.

It'll be in the next release.

-- 
Martin

On 19 Mar 2002, Paul Haas <paulh@hamjudo.com>
wrote:
> I filed this as bug 4150 http://rsync.samba.org/cgi-bin/rsync/
Jitterbug, good, but I don't think anybody is really maintaining
it at the moment.  (For example, there are 733 messages in
"incoming".)

I wonder if it would be better to just disable it and rely on the
mailing list and TODO file?

The Faq-O-Matic seems to be working reasonably well, because people
are really actively answering questions and pruning questions.

-- 
Martin

The fix for this bug is incomplete: rsync can still do kill(-1, SIGUSR2) ...

On Mar 19, 1:34pm, paulh@hamjudo.com ("Paul Haas") wrote:
-- Subject: Re: [PATCH] rsync kills all user processes on fork
failure
>
> I filed this as bug 4150 http://rsync.samba.org/cgi-bin/rsync/
> 
> We had a few hundred hung processes and reached the limit for "max
user
> processes" (see the ulimit command).   rsync did not handle this very
> well.  It passed -1 as a PID to kill().  When kill() gets -1, it kills
> all of the processes owned by that user, or if run by root, all processes.
> 
> I attached a one line fix.  Let me know if I used the wrong format for a
> patch.  If the result of fork is -1 don't put it in the list of pids to
> kill.
> 
> Reviewing the mailing list, I saw that this happened to at least one other
> person, see http://lists.samba.org/pipermail/rsync/2001-April/004158.html
> Back in April of 2001, Randy Kramer wrote: ...
>
-- End of excerpt from "Paul Haas"

Actually, I submitted a bug report (with a patch) for this back in
September 2000; see bug 2762 ...

http://rsync.samba.org/cgi-bin/rsync/incoming?id=2762;user=guest;selectid=2762

Could someone look at the original patch?  It shows that there are TWO
distinct kill(-1, ...) bugs, one that affects SIGUSR1 and another that
affects SIGUSR2.  It looks like only one of these (SIGUSR1) has been
fixed ...  The patch for main.c is still needed.

Also, nothing currently protects against overflow of the all_pids array
by do_fork(), although I doubt that there are enough child processes
to trigger this bug (yet).

Bob Byrnes                        e-mail: byrnes@curl.com
Curl Corporation                  phone:  617-761-1200
400 Technology Square, 8th Floor  fax:    617-761-1201
Cambridge, MA  02139