Re: [PATCH]unused NULL check before calling free
On Sat, Aug 02, 2014 at 10:35:43PM +0200, Fritjof Bornebusch wrote:
Ping?
On Fri, Aug 01, 2014 at 08:03:58AM -0400, Ted Unangst wrote:
Half true. :)
The behavior is intended. I don't really know why they care about
freeing null, but the intention is clearly to check for it; otherwise
they would just call free() in the first place. (actually, i think the
rationale is that to assure portability, you need a strict version of
free that matches the broken version found elsewhere or you will
accidentally do something not portable. under that assumption, and
assuming we don't care about those platforms, the correct fix is to
delete xfree entirely.)
The X option, though, to malloc.conf doesn't check NULL to free. It
just makes malloc abort instead of returning null. I don't think it's
a useful option either way. Don't use it.
Based on Teds suggestion, this diff deletes xfree() entirely.
fritjof
Index: xmalloc.h
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.h,v
retrieving revision 1.1
diff -u -p -r1.1 xmalloc.h
--- xmalloc.h 26 Apr 2006 02:55:13 - 1.1
+++ xmalloc.h 2 Aug 2014 20:09:55 -
@@ -22,7 +22,6 @@
void *xmalloc(size_t);
void *xcalloc(size_t, size_t);
void *xrealloc(void *, size_t, size_t);
-void xfree(void *);
char *xstrdup(const char *);
int xasprintf(char **, const char *, ...)
__attribute__((__format__ (printf, 2, 3)))
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 2 Aug 2014 20:10:08 -
@@ -73,14 +73,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
return new_ptr;
}
-void
-xfree(void *ptr)
-{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
- free(ptr);
-}
-
char *
xstrdup(const char *str)
{
Index: ci.c
===
RCS file: /cvs/src/usr.bin/rcs/ci.c,v
retrieving revision 1.217
diff -u -p -r1.217 ci.c
--- ci.c 19 May 2014 19:42:24 - 1.217
+++ ci.c 2 Aug 2014 20:12:30 -
@@ -208,8 +208,7 @@ checkin_main(int argc, char **argv)
printf(%s\n, rcs_version);
exit(0);
case 'w':
- if (pb.author != NULL)
- xfree(pb.author);
+ free(pb.author);
pb.author = xstrdup(rcs_optarg);
break;
case 'x':
@@ -376,10 +375,9 @@ out:
buf_free(b2);
if (b3 != NULL)
buf_free(b3);
- if (path1 != NULL)
- xfree(path1);
- if (path2 != NULL)
- xfree(path2);
+
+ free(path1);
+ free(path2);
return (NULL);
}
@@ -511,7 +509,7 @@ checkin_update(struct checkin_params *pb
fprintf(stderr,
reuse log message of previous file? [yn](y): );
if (rcs_yesno('y') != 'y') {
- xfree(pb-rcs_msg);
+ free(pb-rcs_msg);
pb-rcs_msg = NULL;
}
}
@@ -584,7 +582,7 @@ checkin_update(struct checkin_params *pb
pb-username, pb-author, NULL, NULL);
if ((pb-flags INTERACTIVE) (pb-rcs_msg[0] == '\0')) {
- xfree(pb-rcs_msg); /* free empty log message */
+ free(pb-rcs_msg); /* free empty log message */
pb-rcs_msg = NULL;
}
@@ -988,25 +986,22 @@ checkin_parsekeyword(char *keystring, RC
(void)xasprintf(datestring, %s %s, tokens[3], tokens[4]);
if ((*date = date_parse(datestring)) == -1)
errx(1, could not parse date);
- xfree(datestring);
+ free(datestring);
if (i 6)
break;
- if (*author != NULL)
- xfree(*author);
+ free(*author);
*author = xstrdup(tokens[5]);
if (i 7)
break;
- if (*state != NULL)
- xfree(*state);
+ free(*state);
*state = xstrdup(tokens[6]);
break;
case KW_TYPE_AUTHOR:
if (i 2)
break;
- if (*author != NULL)
- xfree(*author);
+ free(*author);
*author = xstrdup(tokens[1]);
break;
case KW_TYPE_DATE:
@@ -1015,13 +1010,12 @@ checkin_parsekeyword(char *keystring, RC
(void)xasprintf(datestring, %s %s, tokens[1], tokens[2]);
if
Re: [PATCH]unused NULL check before calling free
On Fri, Aug 01, 2014 at 08:03:58AM -0400, Ted Unangst wrote:
Half true. :)
The behavior is intended. I don't really know why they care about
freeing null, but the intention is clearly to check for it; otherwise
they would just call free() in the first place. (actually, i think the
rationale is that to assure portability, you need a strict version of
free that matches the broken version found elsewhere or you will
accidentally do something not portable. under that assumption, and
assuming we don't care about those platforms, the correct fix is to
delete xfree entirely.)
The X option, though, to malloc.conf doesn't check NULL to free. It
just makes malloc abort instead of returning null. I don't think it's
a useful option either way. Don't use it.
Based on Teds suggestion, this diff deletes xfree() entirely.
fritjof
Index: xmalloc.h
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.h,v
retrieving revision 1.1
diff -u -p -r1.1 xmalloc.h
--- xmalloc.h 26 Apr 2006 02:55:13 - 1.1
+++ xmalloc.h 2 Aug 2014 20:09:55 -
@@ -22,7 +22,6 @@
void *xmalloc(size_t);
void *xcalloc(size_t, size_t);
void *xrealloc(void *, size_t, size_t);
-void xfree(void *);
char *xstrdup(const char *);
int xasprintf(char **, const char *, ...)
__attribute__((__format__ (printf, 2, 3)))
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 2 Aug 2014 20:10:08 -
@@ -73,14 +73,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
return new_ptr;
}
-void
-xfree(void *ptr)
-{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
- free(ptr);
-}
-
char *
xstrdup(const char *str)
{
Index: ci.c
===
RCS file: /cvs/src/usr.bin/rcs/ci.c,v
retrieving revision 1.217
diff -u -p -r1.217 ci.c
--- ci.c19 May 2014 19:42:24 - 1.217
+++ ci.c2 Aug 2014 20:12:30 -
@@ -208,8 +208,7 @@ checkin_main(int argc, char **argv)
printf(%s\n, rcs_version);
exit(0);
case 'w':
- if (pb.author != NULL)
- xfree(pb.author);
+ free(pb.author);
pb.author = xstrdup(rcs_optarg);
break;
case 'x':
@@ -376,10 +375,9 @@ out:
buf_free(b2);
if (b3 != NULL)
buf_free(b3);
- if (path1 != NULL)
- xfree(path1);
- if (path2 != NULL)
- xfree(path2);
+
+ free(path1);
+ free(path2);
return (NULL);
}
@@ -511,7 +509,7 @@ checkin_update(struct checkin_params *pb
fprintf(stderr,
reuse log message of previous file? [yn](y): );
if (rcs_yesno('y') != 'y') {
- xfree(pb-rcs_msg);
+ free(pb-rcs_msg);
pb-rcs_msg = NULL;
}
}
@@ -584,7 +582,7 @@ checkin_update(struct checkin_params *pb
pb-username, pb-author, NULL, NULL);
if ((pb-flags INTERACTIVE) (pb-rcs_msg[0] == '\0')) {
- xfree(pb-rcs_msg); /* free empty log message */
+ free(pb-rcs_msg); /* free empty log message */
pb-rcs_msg = NULL;
}
@@ -988,25 +986,22 @@ checkin_parsekeyword(char *keystring, RC
(void)xasprintf(datestring, %s %s, tokens[3], tokens[4]);
if ((*date = date_parse(datestring)) == -1)
errx(1, could not parse date);
- xfree(datestring);
+ free(datestring);
if (i 6)
break;
- if (*author != NULL)
- xfree(*author);
+ free(*author);
*author = xstrdup(tokens[5]);
if (i 7)
break;
- if (*state != NULL)
- xfree(*state);
+ free(*state);
*state = xstrdup(tokens[6]);
break;
case KW_TYPE_AUTHOR:
if (i 2)
break;
- if (*author != NULL)
- xfree(*author);
+ free(*author);
*author = xstrdup(tokens[1]);
break;
case KW_TYPE_DATE:
@@ -1015,13 +1010,12 @@ checkin_parsekeyword(char *keystring, RC
(void)xasprintf(datestring, %s %s, tokens[1], tokens[2]);
if ((*date = date_parse(datestring)) == -1)
Re: [PATCH]unused NULL check before calling free
On Thu, Jul 31, 2014 at 18:10, Remco wrote:
Fritjof Bornebusch wrote:
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Looking for xmalloc/xfree on the internet it seems this behaviour is
intended. On OpenBSD at least malloc(3) has a reference to xmalloc by means
of its 'X' malloc option.
Half true. :)
The behavior is intended. I don't really know why they care about
freeing null, but the intention is clearly to check for it; otherwise
they would just call free() in the first place. (actually, i think the
rationale is that to assure portability, you need a strict version of
free that matches the broken version found elsewhere or you will
accidentally do something not portable. under that assumption, and
assuming we don't care about those platforms, the correct fix is to
delete xfree entirely.)
The X option, though, to malloc.conf doesn't check NULL to free. It
just makes malloc abort instead of returning null. I don't think it's
a useful option either way. Don't use it.
Re: [PATCH]unused NULL check before calling free
On Wed, Jul 30, 2014 at 07:37:29PM -0700, patrick keshishian wrote:
On Wed, Jul 30, 2014 at 10:14:54PM +0200, Fritjof Bornebusch wrote:
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Going by this context, a quick grep in src/usr.bin/rcs, and
especially the error message, the NULL check's purpose is to
catach this condition.
The code you claim to correct:
there is an unnecessary NULL check before calling free.
would have been of the form:
if (ptr != NULL)-or-if (ptr == NULL)
free(ptr); return;
That is true, but free() can handle NULL itself:
...
/* This is legal. */
if (ptr == NULL)
return;
That's why there is no need, to use a condition like:
if(ptr == NULL)
errx(1, xfree: NULL pointer given as argument);
--patrick
fritjof
Re: [PATCH]unused NULL check before calling free
On Thu, Jul 31, 2014 at 6:39 AM, Fritjof Bornebusch frit...@alokat.org wrote:
On Wed, Jul 30, 2014 at 07:37:29PM -0700, patrick keshishian wrote:
On Wed, Jul 30, 2014 at 10:14:54PM +0200, Fritjof Bornebusch wrote:
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Going by this context, a quick grep in src/usr.bin/rcs, and
especially the error message, the NULL check's purpose is to
catach this condition.
The code you claim to correct:
there is an unnecessary NULL check before calling free.
would have been of the form:
if (ptr != NULL)-or-if (ptr == NULL)
free(ptr); return;
That is true, but free() can handle NULL itself:
...
/* This is legal. */
if (ptr == NULL)
return;
That's why there is no need, to use a condition like:
if(ptr == NULL)
errx(1, xfree: NULL pointer given as argument);
--patrick
fritjof
For educational purpose i guess humanity need a free(NULL) is valid T shirt,
i learned it in the mailing list around a year ago.
Maybe the back would be if not, patch your OS, or better use OpenBSD
--
-
() ascii ribbon campaign - against html e-mail
/\
Re: [PATCH]unused NULL check before calling free
On Thu, Jul 31, 2014 at 10:32:07AM -0400, sven falempin wrote:
On Thu, Jul 31, 2014 at 6:39 AM, Fritjof Bornebusch frit...@alokat.org
wrote:
On Wed, Jul 30, 2014 at 07:37:29PM -0700, patrick keshishian wrote:
On Wed, Jul 30, 2014 at 10:14:54PM +0200, Fritjof Bornebusch wrote:
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Going by this context, a quick grep in src/usr.bin/rcs, and
especially the error message, the NULL check's purpose is to
catach this condition.
The code you claim to correct:
there is an unnecessary NULL check before calling free.
would have been of the form:
if (ptr != NULL)-or-if (ptr == NULL)
free(ptr); return;
That is true, but free() can handle NULL itself:
...
/* This is legal. */
if (ptr == NULL)
return;
That's why there is no need, to use a condition like:
if(ptr == NULL)
errx(1, xfree: NULL pointer given as argument);
--patrick
fritjof
For educational purpose i guess humanity need a free(NULL) is valid T shirt,
i learned it in the mailing list around a year ago.
Maybe the back would be if not, patch your OS, or better use OpenBSD
That would be very cool!
--
-
() ascii ribbon campaign - against html e-mail
/\
Re: [PATCH]unused NULL check before calling free
On 07/31/14 12:39, Fritjof Bornebusch wrote: That's why there is no need, to use a condition like: if(ptr == NULL) errx(1, xfree: NULL pointer given as argument); Yes, there is. There is a big difference between free(NULL) and xfree(NULL).
[PATCH]unused NULL check before calling free
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Re: [PATCH]unused NULL check before calling free
On Wed, Jul 30, 2014 at 10:14:54PM +0200, Fritjof Bornebusch wrote:
Hi tech,
there is an unnecessary NULL check before calling free.
fritjof
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.4
diff -u -p -r1.4 xmalloc.c
--- xmalloc.c 7 Jun 2009 08:39:13 - 1.4
+++ xmalloc.c 31 May 2014 19:19:18 -
@@ -76,8 +76,6 @@ xrealloc(void *ptr, size_t nmemb, size_t
void
xfree(void *ptr)
{
- if (ptr == NULL)
- errx(1, xfree: NULL pointer given as argument);
free(ptr);
}
Going by this context, a quick grep in src/usr.bin/rcs, and
especially the error message, the NULL check's purpose is to
catach this condition.
The code you claim to correct:
there is an unnecessary NULL check before calling free.
would have been of the form:
if (ptr != NULL)-or-if (ptr == NULL)
free(ptr); return;
--patrick
