Plug a mem-leak in dhcpd(8)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Plug a mem-leak in dhcpd(8)

Michael McConville-2
Am I interpreting this correctly?

This is the least invasive fix, but it's unfortunate that this function
allows the supplied buffer to be NULL. If we made it unconditionally
allocate a new buffer, we would have to change some program logic
because uses pass stack-allocated statically-sized buffers. So, maybe
should require a valid buffer argument.


Index: parse.c
===================================================================
RCS file: /cvs/src/usr.sbin/dhcpd/parse.c,v
retrieving revision 1.18
diff -u -p -r1.18 parse.c
--- parse.c 18 May 2015 17:51:21 -0000 1.18
+++ parse.c 23 Sep 2015 20:03:56 -0000
@@ -41,6 +41,7 @@
  */
 
 #include  <stdint.h>
+#include  <stdbool.h>
 
 #include "dhcpd.h"
 #include "dhctoken.h"
@@ -287,11 +288,13 @@ parse_numeric_aggregate(FILE *cfile, uns
  int token, count = 0;
  unsigned char *bufp = buf, *s = NULL;
  pair c = NULL;
+ bool newbufp = false;
 
  if (!bufp && *max) {
  bufp = malloc(*max * size / 8);
  if (!bufp)
  error("can't allocate space for numeric aggregate");
+ newbufp = true;
  } else
  s = bufp;
 
@@ -306,6 +309,8 @@ parse_numeric_aggregate(FILE *cfile, uns
  parse_warn("too few numbers.");
  if (token != ';')
  skip_to_semi(cfile);
+ if (newbufp)
+ free(bufp);
  return (NULL);
  }
  token = next_token(&val, cfile);
@@ -319,6 +324,8 @@ parse_numeric_aggregate(FILE *cfile, uns
  if (token != TOK_NUMBER && token != TOK_NUMBER_OR_NAME) {
  parse_warn("expecting numeric value.");
  skip_to_semi(cfile);
+ if (newbufp)
+ free(bufp);
  return (NULL);
  }
  /*
@@ -338,6 +345,8 @@ parse_numeric_aggregate(FILE *cfile, uns
 
  /* If we had to cons up a list, convert it now. */
  if (c) {
+ if (newbufp)
+ free(bufp);
  bufp = malloc(count * size / 8);
  if (!bufp)
  error("can't allocate space for numeric aggregate.");

Reply | Threaded
Open this post in threaded view
|

Re: Plug a mem-leak in dhcpd(8)

Ted Unangst-6
Michael McConville wrote:
> Am I interpreting this correctly?
>
> This is the least invasive fix, but it's unfortunate that this function
> allows the supplied buffer to be NULL. If we made it unconditionally
> allocate a new buffer, we would have to change some program logic
> because uses pass stack-allocated statically-sized buffers. So, maybe
> should require a valid buffer argument.

One or the other. This idiom of allowing the caller to pass a buffer or not
and then allocating memory or not is silly and a source of many bugs. I don't
believe we should encourage its use by demonstrating it except where
necessary.