Date: Thu, 12 Jul 2001 01:09:08 -0700 (PDT) From: Matt Dillon <dillon@earth.backplane.com> To: Dima Dorfman <dima@unixfreak.org> Cc: Mark Peek <mark@whistle.com>, stable@FreeBSD.ORG Subject: Re: cron exited on signal 11 Message-ID: <200107120809.f6C898Q42216@earth.backplane.com> References: <20010712065159.619703E2F@bazooka.unixfreak.org>
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.
Well, I think I disagree here... the eof label does a 'general cleanup'.
It is entirely appropriate for it to call free_entry() here, as long
as free_entry() is designed to handle a partially instantiated structure.
This is a fairly typical programming practice used to avoid having to
do piecemeal unwinds of partially instantiated structures all over the
code (which is what you did in your patch). So I believe the
proper solution is to imply make it so that free_entry() can deal with
a partially instantiated structure - a one line fix.
The eof label itself may not be good programming practice, but the
concept of a general cleanup at the end of a procedure or in case of
error is sound.
-Matt
:
: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?200107120809.f6C898Q42216>