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)
