Re: calendar in Debian
On Sat, Jan 21, 2012 at 09:39:30AM +0100, Otto Moerbeek wrote: On Fri, Jan 20, 2012 at 10:48:54AM +0100, Otto Moerbeek wrote: On Fri, Jan 20, 2012 at 10:38:35AM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 09:41:08PM +0100, Otto Moerbeek wrote: With a hint from Paul Jantzen I did test this a bit further. There's That is, Paul Janzen, sorry about that. code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time After reading Paul's explanation I found my mistake. The fix works great for calendar -a but not if you call calendar directly on the named pipe. Of course the latter is not really a problem, so I'm going to remove that patch from our sources. Thanks for the explanations. Any ideas about the other patches? I don't think I have a chance to look at them the coming time. Any other volunteer here? -Otto Buete here is Paul's diff. It makes sure all descendants are killed, insted of only the direct child. OK? Nobody? Unless somebody objects, I'm going to commit this soon. -Otto -Otto Index: calendar.c === RCS file: /cvs/src/usr.bin/calendar/calendar.c,v retrieving revision 1.27 diff -u calendar.c --- calendar.c12 Sep 2011 21:23:00 - 1.27 +++ calendar.c19 Jan 2012 18:29:22 - @@ -169,6 +169,7 @@ warn(fork); continue; case 0: /* child */ + (void)setpgid(getpid(), getpid()); (void)setlocale(LC_ALL, ); if (setusercontext(NULL, pw, pw-pw_uid, LOGIN_SETALL ^ LOGIN_SETLOGIN)) @@ -214,7 +215,10 @@ /* It doesn't _really_ matter if the kill fails, e.g. * if there's only a zombie now. */ - (void)kill(kid, SIGTERM); + if (getpgid(kid) != getpgrp()) + (void)killpg(getpgid(kid), SIGTERM); + else + (void)kill(kid, SIGTERM); warnx(uid %u did not finish in time, pw-pw_uid); } if (time(NULL) - t = SECSPERDAY)
Re: calendar in Debian
On Fri, Jan 20, 2012 at 10:48:54AM +0100, Otto Moerbeek wrote: On Fri, Jan 20, 2012 at 10:38:35AM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 09:41:08PM +0100, Otto Moerbeek wrote: With a hint from Paul Jantzen I did test this a bit further. There's That is, Paul Janzen, sorry about that. code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time After reading Paul's explanation I found my mistake. The fix works great for calendar -a but not if you call calendar directly on the named pipe. Of course the latter is not really a problem, so I'm going to remove that patch from our sources. Thanks for the explanations. Any ideas about the other patches? I don't think I have a chance to look at them the coming time. Any other volunteer here? -Otto Buete here is Paul's diff. It makes sure all descendants are killed, insted of only the direct child. OK? -Otto Index: calendar.c === RCS file: /cvs/src/usr.bin/calendar/calendar.c,v retrieving revision 1.27 diff -u calendar.c --- calendar.c 12 Sep 2011 21:23:00 - 1.27 +++ calendar.c 19 Jan 2012 18:29:22 - @@ -169,6 +169,7 @@ warn(fork); continue; case 0: /* child */ + (void)setpgid(getpid(), getpid()); (void)setlocale(LC_ALL, ); if (setusercontext(NULL, pw, pw-pw_uid, LOGIN_SETALL ^ LOGIN_SETLOGIN)) @@ -214,7 +215,10 @@ /* It doesn't _really_ matter if the kill fails, e.g. * if there's only a zombie now. */ - (void)kill(kid, SIGTERM); + if (getpgid(kid) != getpgrp()) + (void)killpg(getpgid(kid), SIGTERM); + else + (void)kill(kid, SIGTERM); warnx(uid %u did not finish in time, pw-pw_uid); } if (time(NULL) - t = SECSPERDAY)
Re: calendar in Debian
On Thu, Jan 19, 2012 at 05:46:48PM +0100, Otto Moerbeek wrote: Introducing a race is never the right solution, imo. That I totally agree with. By the way, your fix does not catch an included file being a fifo either. So while introducing a race, it actually does not fix the problem. Good point. It seems noone noticed so far. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: calendar in Debian
On Fri, Jan 20, 2012 at 10:30:01AM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 09:38:06PM +0100, Otto Moerbeek wrote: With a hint from Paul Jantzen I did test this a bit further. There's code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time Now that's interesting. I certainly don't see this message. Here's what I did: $ mkfifo c $ ./calendar -f c I killed the program after 40 minutes without any action from it, strace verifies it still waits in open(). Could it be that this works differently on OpenBSD? Yes indeed. Likely we diverged here from net and freebsd. In particular, rev 1.15 op calendar.c and rev 1.13 of io.c are relevant. BTW, there's a slight problem with the current code that kills the child after a timeout. Paul Janzen sent me a diff I'm currently testing. -Otto
Re: calendar in Debian
On Thu, Jan 19, 2012 at 09:41:08PM +0100, Otto Moerbeek wrote: With a hint from Paul Jantzen I did test this a bit further. There's That is, Paul Janzen, sorry about that. code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time After reading Paul's explanation I found my mistake. The fix works great for calendar -a but not if you call calendar directly on the named pipe. Of course the latter is not really a problem, so I'm going to remove that patch from our sources. Thanks for the explanations. Any ideas about the other patches? Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: calendar in Debian
On Fri, Jan 20, 2012 at 10:38:35AM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 09:41:08PM +0100, Otto Moerbeek wrote: With a hint from Paul Jantzen I did test this a bit further. There's That is, Paul Janzen, sorry about that. code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time After reading Paul's explanation I found my mistake. The fix works great for calendar -a but not if you call calendar directly on the named pipe. Of course the latter is not really a problem, so I'm going to remove that patch from our sources. Thanks for the explanations. Any ideas about the other patches? I don't think I have a chance to look at them the coming time. Any other volunteer here? -Otto
Re: calendar in Debian
On Thu, Jan 19, 2012 at 10:40:13AM +0100, Michael Meskes wrote: Hi, I'm working on the Debian package bsdmainutils which includes calendar from OpenBSD. In an effort to fix bugs and improve the feature set we added several patches to calendar. Some are Linux specific, but the majority would also be applicable to OpenBSD. Maybe you're interested in adding these changes to your sources. You can find the patches here: http://anonscm.debian.org/gitweb/?p=bsdmainutils/bsdmainutils.git;a=tree;f=debian/patches;hb=HEAD I'm absolutely willing to explain what we did with each patch and why. I looked through some of the diffs and noted your are introducing a race in calendar_fifo.diff. Why are you changing this code? -Otto Feel free to redirect me or forward this email to the right place/person if this list is not the right place. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: calendar in Debian
On Thu, Jan 19, 2012 at 12:30:15PM +0100, Otto Moerbeek wrote: I looked through some of the diffs and noted your are introducing a race in calendar_fifo.diff. Why are you changing this code? Because the old code blocks if a named pipe is used as a calendar file, a bug report is here: http://bugs.launchpad.net/ubuntu/+source/bsdmainutils/+bug/357055 Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL
Re: calendar in Debian
On Thu, Jan 19, 2012 at 05:46:48PM +0100, Otto Moerbeek wrote: On Thu, Jan 19, 2012 at 05:09:25PM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 12:30:15PM +0100, Otto Moerbeek wrote: I looked through some of the diffs and noted your are introducing a race in calendar_fifo.diff. Why are you changing this code? Because the old code blocks if a named pipe is used as a calendar file, a bug report is here: http://bugs.launchpad.net/ubuntu/+source/bsdmainutils/+bug/357055 Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL Introducing a race is never the right solution, imo. By the way, your fix does not catch an included file being a fifo either. So while introducing a race, it actually does not fix the problem. With a hint from Paul Jantzen I did test this a bit further. There's code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time So imo, the debian diff not only is wrong, but it also tries to solve a problem that isn't actually a problem. -Otto
Re: calendar in Debian
On Thu, Jan 19, 2012 at 09:38:06PM +0100, Otto Moerbeek wrote: On Thu, Jan 19, 2012 at 05:46:48PM +0100, Otto Moerbeek wrote: On Thu, Jan 19, 2012 at 05:09:25PM +0100, Michael Meskes wrote: On Thu, Jan 19, 2012 at 12:30:15PM +0100, Otto Moerbeek wrote: I looked through some of the diffs and noted your are introducing a race in calendar_fifo.diff. Why are you changing this code? Because the old code blocks if a named pipe is used as a calendar file, a bug report is here: http://bugs.launchpad.net/ubuntu/+source/bsdmainutils/+bug/357055 Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! ForC'a BarC'a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL Introducing a race is never the right solution, imo. By the way, your fix does not catch an included file being a fifo either. So while introducing a race, it actually does not fix the problem. With a hint from Paul Jantzen I did test this a bit further. There's That is, Paul Janzen, sorry about that. code to avoid having a child runing too long. If you have 20s patience, you'll see this: calendar: uid 1000 did not finish in time So imo, the debian diff not only is wrong, but it also tries to solve a problem that isn't actually a problem. -Otto
