Re: cron: NULL pointer dereference in load_entry()
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
Thanks,
Lawrence
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 - 1.32
+++ entry.c 10 May 2011 00:33:11 -
@@ -57,9 +57,12 @@ static int get_list(bitstr_t *, int, int
void
free_entry(entry *e) {
- free(e-cmd);
- free(e-pwd);
- env_free(e-envp);
+ if (e-cmd)
+ free(e-cmd);
+ if (e-pwd)
+ free(e-pwd);
+ if (e-envp)
+ env_free(e-envp);
free(e);
}
@@ -103,6 +106,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 +394,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)
Re: cron: NULL pointer dereference in load_entry()
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 - 1.61
+++ crontab.c 11 May 2011 01:40:11 -
@@ -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 - 1.32
+++ entry.c 11 May 2011 01:40:11 -
@@ -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)
Re: cron: NULL pointer dereference in load_entry()
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 - 1.61
+++ crontab.c 11 May 2011 01:40:11 -
@@ -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 - 1.32
+++ entry.c 11 May 2011 01:40:11 -
@@ -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
cron: NULL pointer dereference in load_entry()
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?
Thanks,
Lawrence
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 - 1.32
+++ entry.c 10 May 2011 00:33:11 -
@@ -57,9 +57,12 @@ static int get_list(bitstr_t *, int, int
void
free_entry(entry *e) {
- free(e-cmd);
- free(e-pwd);
- env_free(e-envp);
+ if (e-cmd)
+ free(e-cmd);
+ if (e-pwd)
+ free(e-pwd);
+ if (e-envp)
+ env_free(e-envp);
free(e);
}
@@ -103,6 +106,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 +394,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)
Re: cron: NULL pointer dereference in load_entry()
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
Thanks,
Lawrence
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 - 1.32
+++ entry.c 10 May 2011 00:33:11 -
@@ -57,9 +57,12 @@ static int get_list(bitstr_t *, int, int
void
free_entry(entry *e) {
- free(e-cmd);
- free(e-pwd);
- env_free(e-envp);
+ if (e-cmd)
+ free(e-cmd);
+ if (e-pwd)
+ free(e-pwd);
+ if (e-envp)
+ env_free(e-envp);
free(e);
}
@@ -103,6 +106,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 +394,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)
