On Tue, May 10, 2011 at 10:01:00PM -0400, Lawrence Teo wrote:
> On Tue, May 10, 2011 at 08:46:04AM -0400, Kenneth R Westerback wrote:
> > On Tue, May 10, 2011 at 06:57:29AM +0200, Otto Moerbeek wrote:
> > > On Mon, May 09, 2011 at 08:51:01PM -0400, Lawrence Teo wrote:
> > >
> > > > In the load_entry() function in cron's entry.c, calloc() is called
> > > > but its return value is never checked to see if it is NULL,
> > > > potentially causing a NULL pointer dereference. Since crontab(1)
> > > > shares code with cron, it is affected as well.
> > > >
> > > > The following diff adds the check for NULL, and also moves the
> > > > cleanup code that addresses this condition to free_entry() in order
> > > > to remove duplicate code.
> > > >
> > > > Any thoughts or comments?
> > >
> > > Have to look in detail for the stuctural changes, but free(NULL) has
> > > ben OK for ages.
> > >
> > > -Otto
> > >
> >
> > Ditto. I think the calloc() return value check is good as e is referenced
> > in following code. But free_entry should be ok if you ensure you don't
> > pass it NULL.
> >
> > .... Ken
>
>
> Otto and Ken,
>
> Thank you for your helpful comments. I have revised the diff
> according to your feedback.
>
> I did include a check in free_entry() to ensure that e->envp is not
> NULL before passing it to env_free(), since env_free(NULL) would
> cause a segfault.
>
> In addition, I modified crontab.c to use free_entry(e) instead of
> free(e). This keeps the cleanup code for e consistent throughout the
> program.
>
> What do you think of the revised diff? Would appreciate your
> thoughts.
>
> Thank you,
> Lawrence
>
>
> Index: crontab.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/crontab.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 crontab.c
> --- crontab.c 4 Apr 2011 15:17:52 -0000 1.61
> +++ crontab.c 11 May 2011 01:40:11 -0000
> @@ -547,7 +547,7 @@ replace_cmd(void) {
> case FALSE:
> e = load_entry(tmp, check_error, pw, envp);
> if (e)
> - free(e);
> + free_entry(e);
> break;
> case TRUE:
> break;
> Index: entry.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/cron/entry.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 entry.c
> --- entry.c 29 Oct 2009 18:56:47 -0000 1.32
> +++ entry.c 11 May 2011 01:40:11 -0000
> @@ -59,7 +59,8 @@ void
> free_entry(entry *e) {
> free(e->cmd);
> free(e->pwd);
> - env_free(e->envp);
> + if (e->envp)
> + env_free(e->envp);
> free(e);
> }
>
> @@ -103,6 +104,10 @@ load_entry(FILE *file, void (*error_func
> */
>
> e = (entry *) calloc(sizeof(entry), sizeof(char));
> + if (e == NULL) {
> + ecode = e_memory;
> + goto eof;
> + }
>
> if (ch == '@') {
> /* all of these should be flagged and load-limited; i.e.,
> @@ -387,13 +392,8 @@ load_entry(FILE *file, void (*error_func
> return (e);
>
> eof:
> - if (e->envp)
> - env_free(e->envp);
> - if (e->pwd)
> - free(e->pwd);
> - if (e->cmd)
> - free(e->cmd);
> - free(e);
> + if (e)
> + free_entry(e);
> while (ch != '\n' && !feof(file))
> ch = get_char(file);
> if (ecode != e_none && error_func)
This seems reasonable at first glance.
.... Ken
