From owner-freebsd-stable Thu Jul 12 1: 9:26 2001 Delivered-To: freebsd-stable@freebsd.org Received: from earth.backplane.com (earth-nat-cw.backplane.com [208.161.114.67]) by hub.freebsd.org (Postfix) with ESMTP id D9AB537B401 for ; Thu, 12 Jul 2001 01:09:14 -0700 (PDT) (envelope-from dillon@earth.backplane.com) Received: (from dillon@localhost) by earth.backplane.com (8.11.3/8.11.2) id f6C898Q42216; Thu, 12 Jul 2001 01:09:08 -0700 (PDT) (envelope-from dillon) Date: Thu, 12 Jul 2001 01:09:08 -0700 (PDT) From: Matt Dillon Message-Id: <200107120809.f6C898Q42216@earth.backplane.com> To: Dima Dorfman Cc: Mark Peek , stable@FreeBSD.ORG Subject: Re: cron exited on signal 11 References: <20010712065159.619703E2F@bazooka.unixfreak.org> Sender: owner-freebsd-stable@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG : :Matt Dillon 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 :> From: Dima Dorfman :> Subject: Re: cron exited on signal 11 :> :> In-Reply-To: ; from mark@whistle.com on :> *** "Wed, 11 Jul 2001 20:27:54 -0700" :> :> Mark Peek 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