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>