Re: [Dnsmasq-discuss] [PATCH] Free config file values on parsing errors.

2018-11-02 Thread Simon Kelley
On 25/10/2018 09:36, Petr Mensik wrote:
> Hi again.
> 
> This time I have a little bit more controversal patches. But I think
> still useful. They fixes memory leaks that might occur in some cases.
> Most dnsmasq errors is fatal, so it does not matter. But some are not.
> Some parts are reloaded on SIGHUP signal, so it might leak more than once.
> 
> Some example when it changes the failures. Use dhcp-options file with
> this content:
> 
> tag:error,vendor:redhat
> option:ntp-server,1.2.3.4.5
> option6:ntp-server,[:::]
> 
> Is not fatal and dnsmasq will start. On each reload command, it would
> leak some memory. I validated it using valgrind --leak-check=full
> dnsmasq -d. This patch fixes it. It introduces something that might be
> considered constructor and destructor of selected structures. What do
> you think of it?
> 
> Comments are welcome. Another patch would be sent short after, they are
> too big together to require moderator attention.
> 

I've applied the patches. I think they're doing the right thing.

I'm a little bit concerned about how extensive the changes are, but I've
done my best to review everything, and I can't find any problems. We're
early in the 2.81 cycle, so if there is anything lurking, there's a
chance to find it.


Cheers,

Simon.


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss


[Dnsmasq-discuss] [PATCH] Free config file values on parsing errors.

2018-10-27 Thread Petr Mensik
Hi again.

This time I have a little bit more controversal patches. But I think
still useful. They fixes memory leaks that might occur in some cases.
Most dnsmasq errors is fatal, so it does not matter. But some are not.
Some parts are reloaded on SIGHUP signal, so it might leak more than once.

Some example when it changes the failures. Use dhcp-options file with
this content:

tag:error,vendor:redhat
option:ntp-server,1.2.3.4.5
option6:ntp-server,[:::]

Is not fatal and dnsmasq will start. On each reload command, it would
leak some memory. I validated it using valgrind --leak-check=full
dnsmasq -d. This patch fixes it. It introduces something that might be
considered constructor and destructor of selected structures. What do
you think of it?

Comments are welcome.

Cheers,
Petr
-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com  PGP: 65C6C973
From 55ecb0728dbd19ef3412be2dc70d2ed9dd6ecc5e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= 
Date: Thu, 16 Aug 2018 15:48:00 +0200
Subject: [PATCH 1/2] More fixes free options

Free resources on failed config file parsing.
Make sure one_opt will free allocated memory on failed parsing.
All unsaved memory is freed on syntax error in options.
---
 src/option.c | 360 ++-
 1 file changed, 257 insertions(+), 103 deletions(-)

diff --git a/src/option.c b/src/option.c
index 56ef945..547d36e 100644
--- a/src/option.c
+++ b/src/option.c
@@ -759,6 +759,7 @@ static void do_usage(void)
 }
 
 #define ret_err(x) do { strcpy(errstr, (x)); return 0; } while (0)
+#define ret_err_free(x,m) do { strcpy(errstr, (x)); free((m)); return 0; } while (0)
 
 static char *parse_mysockaddr(char *arg, union mysockaddr *addr) 
 {
@@ -904,6 +905,8 @@ static struct server *add_rev4(struct in_addr addr, int msize)
   p += sprintf(p, "%d.", (a >> 24) & 0xff);
   break;
 default:
+  free(serv->domain);
+  free(serv);
   return NULL;
 }
 
@@ -1069,7 +1072,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
   if (is6)
 {
   if (new->flags & (DHOPT_VENDOR | DHOPT_ENCAPSULATE))
-	ret_err(_("unsupported encapsulation for IPv6 option"));
+	ret_err_free(_("unsupported encapsulation for IPv6 option"), new);
   
   if (opt_len == 0 &&
 	  !(new->flags & DHOPT_RFC3925))
@@ -1083,7 +1086,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
   
   /* option may be missing with rfc3925 match */
   if (!option_ok)
-ret_err(_("bad dhcp-option"));
+ret_err_free(_("bad dhcp-option"), new);
   
   if (comma)
 {
@@ -1151,10 +1154,10 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 	  is_string = is_dec = is_hex = 0;
 	  
 	  if (!is6 && (!is_addr || dots == 0))
-	ret_err(_("bad IP address"));
+	ret_err_free(_("bad IP address"), new);
 
 	   if (is6 && !is_addr6)
-	 ret_err(_("bad IPv6 address"));
+	 ret_err_free(_("bad IPv6 address"), new);
 	}
   /* or names */
   else if (opt_len & (OT_NAME | OT_RFC1035_NAME | OT_CSTRING))
@@ -1247,7 +1250,10 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 	  comma = split(cp);
 	  slash = split_chr(cp, '/');
 	  if (!inet_pton(AF_INET, cp, ))
-		ret_err(_("bad IPv4 address"));
+{
+  free(new->val);
+		  ret_err_free(_("bad IPv4 address"), new);
+}
 	  if (!slash)
 		{
 		  memcpy(op, , INADDRSZ);
@@ -1292,8 +1298,9 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 		  op += IN6ADDRSZ;
 		  continue;
 		}
-	  
-	  ret_err(_("bad IPv6 address"));
+
+  free(new->val);
+	  ret_err_free(_("bad IPv6 address"), new);
 	} 
 	  new->len = op - new->val;
 	}
@@ -1320,7 +1327,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 		  if (strcmp (arg, ".") != 0)
 		{
 		  if (!(dom = canonicalise_opt(arg)))
-			ret_err(_("bad domain in dhcp-option"));
+			ret_err_free(_("bad domain in dhcp-option"), new);
 			
 		  domlen = strlen(dom) + 2;
 		}
@@ -1414,7 +1421,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 		{
 		  char *dom = canonicalise_opt(arg);
 		  if (!dom)
-		ret_err(_("bad domain in dhcp-option"));
+		ret_err_free(_("bad domain in dhcp-option"), new);
 				  
 		  newp = opt_malloc(len + strlen(dom) + 2);
 		  
@@ -1452,14 +1459,14 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
   ((new->len > 255) || 
   (new->len > 253 && (new->flags & (DHOPT_VENDOR | DHOPT_ENCAPSULATE))) ||
(new->len > 250 && (new->flags & DHOPT_RFC3925
-ret_err(_("dhcp-option too long"));
+ret_err_free(_("dhcp-option too long"), new);
   
   if (flags == DHOPT_MATCH)
 {
   if ((new->flags & (DHOPT_ENCAPSULATE | DHOPT_VENDOR)) ||
 	  !new->netid ||
 	  new->netid->next)
-	ret_err(_("illegal dhcp-match"));
+	ret_err_free(_("illegal 

Re: [Dnsmasq-discuss] [PATCH] Free config file values on parsing errors.

2018-10-26 Thread Petr Mensik
Additional patch that reduces some repeating parts.

On 10/25/2018 10:36 AM, Petr Mensik wrote:
> Hi again.
> 
> This time I have a little bit more controversal patches. But I think
> still useful. They fixes memory leaks that might occur in some cases.
> Most dnsmasq errors is fatal, so it does not matter. But some are not.
> Some parts are reloaded on SIGHUP signal, so it might leak more than once.
> 
> Some example when it changes the failures. Use dhcp-options file with
> this content:
> 
> tag:error,vendor:redhat
> option:ntp-server,1.2.3.4.5
> option6:ntp-server,[:::]
> 
> Is not fatal and dnsmasq will start. On each reload command, it would
> leak some memory. I validated it using valgrind --leak-check=full
> dnsmasq -d. This patch fixes it. It introduces something that might be
> considered constructor and destructor of selected structures. What do
> you think of it?
> 
> Comments are welcome. Another patch would be sent short after, they are
> too big together to require moderator attention.
> 
> Cheers,
> Petr
> 
> 
> 
> ___
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss@lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
> 

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com  PGP: 65C6C973
>From ae3837cabc7e7b2fbd2d875f403a3f26a4d81422 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= 
Date: Thu, 16 Aug 2018 18:10:25 +0200
Subject: [PATCH 2/2] Reduce repeating code parsing tags

DHCP parameters often can have optional tags before it. Reduce repeated
parsing of it, use dhcp_tags() to parse and free if unsuccessful.
Make sure null pointer will not crash free function.
Free also errors in dhcp-range.
---
 src/option.c | 326 +++
 1 file changed, 171 insertions(+), 155 deletions(-)

diff --git a/src/option.c b/src/option.c
index 547d36e..66847fb 100644
--- a/src/option.c
+++ b/src/option.c
@@ -577,14 +577,15 @@ static void *opt_malloc(size_t size)
   return ret;
 }
 
-static char *opt_string_alloc(char *cp)
+static char *opt_string_alloc(const char *cp)
 {
   char *ret = NULL;
+  size_t len;
   
-  if (cp && strlen(cp) != 0)
+  if (cp && (len = strlen(cp)) != 0)
 {
-  ret = opt_malloc(strlen(cp)+1);
-  strcpy(ret, cp); 
+  ret = opt_malloc(len+1);
+  memcpy(ret, cp, len+1); 
   
   /* restore hidden metachars */
   unhide_metas(ret);
@@ -760,6 +761,7 @@ static void do_usage(void)
 
 #define ret_err(x) do { strcpy(errstr, (x)); return 0; } while (0)
 #define ret_err_free(x,m) do { strcpy(errstr, (x)); free((m)); return 0; } while (0)
+#define goto_err(x) do { strcpy(errstr, (x)); goto on_error; } while (0)
 
 static char *parse_mysockaddr(char *arg, union mysockaddr *addr) 
 {
@@ -961,6 +963,97 @@ static char *set_prefix(char *arg)
return arg;
 }
 
+static struct dhcp_netid *
+dhcp_netid_create(const char *net, struct dhcp_netid *next)
+{
+  struct dhcp_netid *tt;
+  tt = opt_malloc(sizeof (struct dhcp_netid));
+  tt->net = opt_string_alloc(net);
+  tt->next = next;
+  return tt;
+}
+
+static void dhcp_netid_free(struct dhcp_netid *nid)
+{
+  while (nid)
+{
+  struct dhcp_netid *tmp = nid;
+  nid = nid->next;
+  free(tmp->net);
+  free(tmp);
+}
+}
+
+/* Parse one or more tag:s before parameters.
+ * Moves arg to the end of tags. */
+static struct dhcp_netid * dhcp_tags(char **arg)
+{
+  struct dhcp_netid *id = NULL;
+
+  while (is_tag_prefix(*arg))
+{
+  char *comma = split(*arg);
+  id = dhcp_netid_create((*arg)+4, id);
+  *arg = comma;
+};
+  if (!*arg)
+{
+  dhcp_netid_free(id);
+  id = NULL;
+}
+  return id;
+}
+
+static void dhcp_netid_list_free(struct dhcp_netid_list *netid)
+{
+  while (netid)
+{
+  struct dhcp_netid_list *tmplist = netid;
+  netid = netid->next;
+  dhcp_netid_free(tmplist->list);
+  free(tmplist);
+}
+}
+
+static void dhcp_config_free(struct dhcp_config *config)
+{
+  if (config)
+{
+  struct hwaddr_config *hwaddr = config->hwaddr;
+  while (hwaddr)
+{
+	  struct hwaddr_config *tmp = hwaddr;
+  hwaddr = hwaddr->next;
+	  free(tmp);
+}
+  dhcp_netid_list_free(config->netid);
+  if (config->flags & CONFIG_CLID)
+free(config->clid);
+  free(config);
+}
+}
+
+static void dhcp_context_free(struct dhcp_context *ctx)
+{
+  if (ctx)
+{
+  dhcp_netid_free(ctx->filter);
+  free(ctx->netid.net);
+  free(ctx->template_interface);
+  free(ctx);
+}
+}
+
+static void dhcp_opt_free(struct dhcp_opt *opt)
+{
+  if (opt->flags & DHOPT_VENDOR)
+free(opt->u.vendor_class);
+  dhcp_netid_free(opt->netid);
+  free(opt->val);
+  free(opt);
+}
+
+
 /* This is too insanely large to keep in-line in the switch */
 static int parse_dhcp_opt(char *errstr, char *arg, int flags)
 {
@@ -968,7 +1061,6 @@ static