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 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 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: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: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? 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: > 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 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
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 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. -Otto
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 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
