Date: Wed, 11 Jul 2001 23:51:59 -0700 From: Dima Dorfman <dima@unixfreak.org> To: Matt Dillon <dillon@earth.backplane.com> Cc: Mark Peek <mark@whistle.com>, stable@FreeBSD.ORG Subject: Re: cron exited on signal 11 Message-ID: <20010712065159.619703E2F@bazooka.unixfreak.org> In-Reply-To: <200107120602.f6C628T41618@earth.backplane.com>; from dillon@earth.backplane.com on "Wed, 11 Jul 2001 23:02:08 -0700 (PDT)"
next in thread | previous in thread | raw e-mail | index | archive | help
Matt Dillon <dillon@earth.backplane.com> writes: > Well this certainly opens up a can of worms! Not your patch, but what > it implies. For example, env_set() assumes envp is not NULL as well. > In fact, most of the code assumes envp is not NULL, and 99% of the time > that is true due to the env_init() calls. I think it should be 100%; why would envp be NULL? > I found at least one case in the code where free_entry() is called with > e->envp == NULL. Perhaps a more correct fix would simply be to have > free_entry() check for e->envp != NULL before calling env_free() rather > then having env_free() check for NULL. On the contrary, I think free_entry() should assume that all members aren't NULL because if they're NULL, it's probably a bug (in which case a core dump is advisable). Below is a copy of an e-mail I sent to Mark Peek on this topic. The only thing I'd like to add is that the patch below addresses two different problems. The first is that free_entry() simply shouldn't be called after the eof: label. The second is that the `class' member is unnecessarily treated inconsistently from the others. They really don't belong in the same patch, because the former is a bugfix while the latter is a style/consistency fix. I think that whether a free()-like routine should check if its argument (or members thereof) are NULL is a religious/philosophical thing. We've had arguments about whether free(3) should crash if its argument is NULL, or if it should just let it go. Furthermore, some programmers religiously rely on free_something() doing the right thing with a half-filled structure, while others prefer to have the routine filling in the structure clean up after itself. I don't want to argue about which approach is better, but since the original author used the latter, I think we should stick to it unless there's a good reason (and there isn't, really). Right now, cron consistently uses the latter approach except for the `class' member of `entry'; that was added by ache@, and he chose to be inconsistent. I think we should just fix the inconsistency. If free_entry() is called somewhere when `envp' is NULL, that's another bug that should be fixed. But again, I'd like to point out that there are two issues here: (1) the bugfix, which is pretty straight-forward (free_entry -> free), and (2) the inconsistency issue, which is less straight-forward. Regards, Dima Dorfman dima@unixfreak.org > Date: Wed, 11 Jul 2001 21:19:07 PDT > To: Mark Peek <mark@whistle.com> > From: Dima Dorfman <dima@hornet.unixfreak.org> > Subject: Re: cron exited on signal 11 > > In-Reply-To: <p0510032db772c73d2f62@[207.76.207.129]>; from mark@whistle.com on > *** "Wed, 11 Jul 2001 20:27:54 -0700" > > Mark Peek <mark@whistle.com> writes: > > Index: usr.sbin/cron/lib/env.c > > =================================================================== > > RCS file: /cvs/freebsd/src/usr.sbin/cron/lib/env.c,v > > retrieving revision 1.9 > > diff -u -r1.9 env.c > > --- usr.sbin/cron/lib/env.c 2000/04/30 15:57:00 1.9 > > +++ usr.sbin/cron/lib/env.c 2001/07/12 03:12:29 > > @@ -41,6 +41,8 @@ > > { > > char **p; > > > > + if (envp == NULL) > > + return; > > for (p = envp; *p; p++) > > free(*p); > > free(envp); > > I think this is the wrong fix; the bug is your/our original change of > free(e) -> free_entry(e) after the eof: label. This routine takes > great care (but not great enough, see below) to make sure that it > doesn't forgot to free() something in this structure before jumping to > eof. There's code like this (entry.c, line 397 of rev. 1.12): > > : ch = get_string(cmd, MAX_COMMAND, file, "\n"); > : > : /* a file without a \n before the EOF is rude, so we'll complain... > : */ > : if (ch == EOF) { > : env_free(e->envp); > : ecode = e_cmd; > : goto eof; > : } > > Notice how it calls env_free() before jumping to eof. There are > similar fragments throughout the routine. `envp', `cmd', and `class' > are the only members of `entry' that need to be free()'d. The routine > does a very good job of free()'ing `envp' and `cmd' when appropriate, > but since `class' is an afterthough (introduced in rev. 1.8), it > doesn't bother with that. > > Thus, I think the right fix is to correct its behavior with regards to > free()'ing `class'. Changing free_entry() (or env_free()) not to free > something if it isn't NULL seems wrong; it will just mask other bugs > (e.g., if there is an `entry' floating around with one of the fields > not filled in; it should crash, but wouldn't with your fix). > > Below is a patch that demonstrates what I mean. I haven't tested it > beyond compiling it, but I think it should work. It isn't very pretty > from the point of view of the source code (those #ifdef LOGIN_CAP's > are evil!), but I think the result is a better product. Also note > that not free()'ing `class' didn't result in a leak because it was > taken care of in free_entry(); however, I think that either all or > none members should be free()'d before jumping to eof. The former > (all) seems to be the better choice. > > Thoughts? > > Thanks, > > Dima Dorfman > dima@unixfreak.org > > P.S. I haven't carefully checked that it doesn't forget to free() > something else. `class' is just the obvious one. > > Index: entry.c > =================================================================== > RCS file: /stl/src/FreeBSD/src/usr.sbin/cron/lib/entry.c,v > retrieving revision 1.12 > diff -u -r1.12 entry.c > --- entry.c 2001/06/13 05:49:37 1.12 > +++ entry.c 2001/07/12 04:17:29 > @@ -70,8 +70,7 @@ > entry *e; > { > #ifdef LOGIN_CAP > - if (e->class != NULL) > - free(e->class); > + free(e->class); > #endif > free(e->cmd); > env_free(e->envp); > @@ -291,6 +290,7 @@ > goto eof; > } > if ((lc = login_getclass(e->class)) == NULL) { > + free(e->class); > ecode = e_class; > goto eof; > } > @@ -300,6 +300,9 @@ > if ((s = strrchr(username, ':')) != NULL) { > *s = '\0'; > if ((grp = getgrnam(s + 1)) == NULL) { > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > ecode = e_group; > goto eof; > } > @@ -307,6 +310,9 @@ > > pw = getpwnam(username); > if (pw == NULL) { > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > ecode = e_username; > goto eof; > } > @@ -319,6 +325,9 @@ > } > > if (pw->pw_expire && time(NULL) >= pw->pw_expire) { > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > ecode = e_username; > goto eof; > } > @@ -332,6 +341,9 @@ > e->envp = env_copy(envp); > if (e->envp == NULL) { > warn("env_copy"); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > ecode = e_mem; > goto eof; > } > @@ -341,6 +353,9 @@ > e->envp = env_set(e->envp, envstr); > if (e->envp == NULL) { > warn("env_set(%s)", envstr); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(prev_env); > ecode = e_mem; > goto eof; > @@ -351,6 +366,9 @@ > e->envp = env_set(e->envp, envstr); > if (e->envp == NULL) { > warn("env_set(%s)", envstr); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(prev_env); > ecode = e_mem; > goto eof; > @@ -361,6 +379,9 @@ > e->envp = env_set(e->envp, envstr); > if (e->envp == NULL) { > warn("env_set(%s)", envstr); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(prev_env); > ecode = e_mem; > goto eof; > @@ -371,6 +392,9 @@ > e->envp = env_set(e->envp, envstr); > if (e->envp == NULL) { > warn("env_set(%s)", envstr); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(prev_env); > ecode = e_mem; > goto eof; > @@ -381,6 +405,9 @@ > e->envp = env_set(e->envp, envstr); > if (e->envp == NULL) { > warn("env_set(%s)", envstr); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(prev_env); > ecode = e_mem; > goto eof; > @@ -399,6 +426,9 @@ > /* a file without a \n before the EOF is rude, so we'll complain... > */ > if (ch == EOF) { > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(e->envp); > ecode = e_cmd; > goto eof; > @@ -409,6 +439,9 @@ > e->cmd = strdup(cmd); > if (e->cmd == NULL) { > warn("strdup(\"%d\")", cmd); > +#ifdef LOGIN_CAP > + free(e->class); > +#endif > env_free(e->envp); > ecode = e_mem; > goto eof; > @@ -420,7 +453,7 @@ > return e; > > eof: > - free_entry(e); > + free(e); > if (ecode != e_none && error_func) > (*error_func)(ecodes[(int)ecode]); > while (ch != EOF && ch != '\n') To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-stable" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010712065159.619703E2F>