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?= <pemen...@redhat.com>
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 int parse_dhcp_opt(char *errstr, char *arg, int flags)
char lenchar = 0, *cp;
int addrs, digs, is_addr, is_addr6, is_hex, is_dec, is_string, dots;
char *comma = NULL;
- struct dhcp_netid *np = NULL;
u16 opt_len = 0;
int is6 = 0;
int option_ok = 0;
@@ -1055,14 +1147,9 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
}
else
{
- new->netid = opt_malloc(sizeof (struct dhcp_netid));
/* allow optional "net:" or "tag:" for consistency */
- if (is_tag_prefix(arg))
- new->netid->net = opt_string_alloc(arg+4);
- else
- new->netid->net = opt_string_alloc(set_prefix(arg));
- new->netid->next = np;
- np = new->netid;
+ const char *name = (is_tag_prefix(arg)) ? arg+4 : set_prefix(arg);
+ new->netid = dhcp_netid_create(name, new->netid);
}
arg = comma;
@@ -1072,7 +1159,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
if (is6)
{
if (new->flags & (DHOPT_VENDOR | DHOPT_ENCAPSULATE))
- ret_err_free(_("unsupported encapsulation for IPv6 option"), new);
+ goto_err(_("unsupported encapsulation for IPv6 option"));
if (opt_len == 0 &&
!(new->flags & DHOPT_RFC3925))
@@ -1086,7 +1173,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
/* option may be missing with rfc3925 match */
if (!option_ok)
- ret_err_free(_("bad dhcp-option"), new);
+ goto_err(_("bad dhcp-option"));
if (comma)
{
@@ -1154,10 +1241,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_free(_("bad IP address"), new);
+ goto_err(_("bad IP address"));
if (is6 && !is_addr6)
- ret_err_free(_("bad IPv6 address"), new);
+ goto_err(_("bad IPv6 address"));
}
/* or names */
else if (opt_len & (OT_NAME | OT_RFC1035_NAME | OT_CSTRING))
@@ -1250,10 +1337,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
comma = split(cp);
slash = split_chr(cp, '/');
if (!inet_pton(AF_INET, cp, &in))
- {
- free(new->val);
- ret_err_free(_("bad IPv4 address"), new);
- }
+ goto_err(_("bad IPv4 address"));
if (!slash)
{
memcpy(op, &in, INADDRSZ);
@@ -1299,8 +1383,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
continue;
}
- free(new->val);
- ret_err_free(_("bad IPv6 address"), new);
+ goto_err(_("bad IPv6 address"));
}
new->len = op - new->val;
}
@@ -1327,7 +1410,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
if (strcmp (arg, ".") != 0)
{
if (!(dom = canonicalise_opt(arg)))
- ret_err_free(_("bad domain in dhcp-option"), new);
+ goto_err(_("bad domain in dhcp-option"));
domlen = strlen(dom) + 2;
}
@@ -1421,7 +1504,7 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
{
char *dom = canonicalise_opt(arg);
if (!dom)
- ret_err_free(_("bad domain in dhcp-option"), new);
+ goto_err(_("bad domain in dhcp-option"));
newp = opt_malloc(len + strlen(dom) + 2);
@@ -1459,14 +1542,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_free(_("dhcp-option too long"), new);
+ goto_err(_("dhcp-option too long"));
if (flags == DHOPT_MATCH)
{
if ((new->flags & (DHOPT_ENCAPSULATE | DHOPT_VENDOR)) ||
!new->netid ||
new->netid->next)
- ret_err_free(_("illegal dhcp-match"), new);
+ goto_err(_("illegal dhcp-match"));
if (is6)
{
@@ -1491,6 +1574,9 @@ static int parse_dhcp_opt(char *errstr, char *arg, int flags)
}
return 1;
+on_error:
+ dhcp_opt_free(new);
+ return 0;
}
#endif
@@ -1521,43 +1607,6 @@ static void server_list_free(struct server *list)
}
}
-static void dhcp_netid_free(struct dhcp_netid *nid)
-{
- while (nid)
- {
- struct dhcp_netid *tmp = nid;
- nid = nid->next;
- free(tmp->net);
- free(tmp);
- }
-}
-
-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)
-{
- 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 int one_opt(int option, char *arg, char *errstr, char *gen_err, int command_line, int servers_only)
{
int i;
@@ -2849,26 +2898,19 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
{
if (is_tag_prefix(arg))
{
- char *net = arg+4;
-
/* ignore empty tag */
- if (*net)
- {
- struct dhcp_netid *tt;
- tt = opt_malloc(sizeof (struct dhcp_netid));
- tt->net = opt_string_alloc(net);
- tt->next = new->filter;
- new->filter = tt;
- }
+ if (arg[4])
+ new->filter = dhcp_netid_create(arg+4, new->filter);
}
else
{
if (new->netid.net)
- ret_err_free(_("only one tag allowed"), new);
- else if (strstr(arg, "set:") == arg)
- new->netid.net = opt_string_alloc(arg+4);
+ {
+ dhcp_context_free(new);
+ ret_err(_("only one tag allowed"));
+ }
else
- new->netid.net = opt_string_alloc(arg);
+ new->netid.net = opt_string_alloc(set_prefix(arg));
}
arg = comma;
}
@@ -2884,7 +2926,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
break;
if (k < 2)
- ret_err_free(_("bad dhcp-range"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("bad dhcp-range"));
+ }
if (inet_pton(AF_INET, a[0], &new->start))
{
@@ -2896,7 +2941,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
else if (strcmp(a[1], "proxy") == 0)
new->flags |= CONTEXT_PROXY;
else if (!inet_pton(AF_INET, a[1], &new->end))
- ret_err_free(_("bad dhcp-range"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("bad dhcp-range"));
+ }
if (ntohl(new->start.s_addr) > ntohl(new->end.s_addr))
{
@@ -2911,7 +2959,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
new->flags |= CONTEXT_NETMASK;
leasepos = 3;
if (!is_same_net(new->start, new->end, new->netmask))
- ret_err_free(_("inconsistent DHCP range"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("inconsistent DHCP range"));
+ }
if (k >= 4 && strchr(a[3], '.') &&
@@ -2925,6 +2976,8 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
#ifdef HAVE_DHCP6
else if (inet_pton(AF_INET6, a[0], &new->start6))
{
+ const char *err = NULL;
+
new->flags |= CONTEXT_V6;
new->prefix = 64; /* default */
new->end6 = new->start6;
@@ -2970,19 +3023,24 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
}
}
- if (new->prefix != 64)
+ if (new->prefix > 64)
{
if (new->flags & CONTEXT_RA)
- ret_err_free(_("prefix length must be exactly 64 for RA subnets"), new);
+ err=(_("prefix length must be exactly 64 for RA subnets"));
else if (new->flags & CONTEXT_TEMPLATE)
- ret_err_free(_("prefix length must be exactly 64 for subnet constructors"), new);
+ err=(_("prefix length must be exactly 64 for subnet constructors"));
}
-
- if (new->prefix < 64)
- ret_err_free(_("prefix length must be at least 64"), new);
+ else if (new->prefix < 64)
+ err=(_("prefix length must be at least 64"));
- if (!is_same_net6(&new->start6, &new->end6, new->prefix))
- ret_err_free(_("inconsistent DHCPv6 range"), new);
+ if (!err && !is_same_net6(&new->start6, &new->end6, new->prefix))
+ err=(_("inconsistent DHCPv6 range"));
+
+ if (err)
+ {
+ dhcp_context_free(new);
+ ret_err(err);
+ }
/* dhcp-range=:: enables DHCP stateless on any interface */
if (IN6_IS_ADDR_UNSPECIFIED(&new->start6) && !(new->flags & CONTEXT_TEMPLATE))
@@ -2993,7 +3051,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
struct in6_addr zero;
memset(&zero, 0, sizeof(zero));
if (!is_same_net6(&zero, &new->start6, new->prefix))
- ret_err_free(_("prefix must be zero with \"constructor:\" argument"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("prefix must be zero with \"constructor:\" argument"));
+ }
}
if (addr6part(&new->start6) > addr6part(&new->end6))
@@ -3005,12 +3066,18 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
}
#endif
else
- ret_err_free(_("bad dhcp-range"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("bad dhcp-range"));
+ }
if (leasepos < k)
{
if (leasepos != k-1)
- ret_err_free(_("bad dhcp-range"), new);
+ {
+ dhcp_context_free(new);
+ ret_err(_("bad dhcp-range"));
+ }
if (strcmp(a[leasepos], "infinite") == 0)
new->lease_time = 0xffffffff;
@@ -3122,14 +3189,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
/* dhcp-host has strange backwards-compat needs. */
else if (strstr(arg, "net:") == arg || strstr(arg, "set:") == arg)
{
- struct dhcp_netid *newtag = opt_malloc(sizeof(struct dhcp_netid));
struct dhcp_netid_list *newlist = opt_malloc(sizeof(struct dhcp_netid_list));
- newtag->net = opt_malloc(strlen(arg + 4) + 1);
newlist->next = new->netid;
new->netid = newlist;
- newlist->list = newtag;
- strcpy(newtag->net, arg+4);
- unhide_metas(newtag->net);
+ newlist->list = dhcp_netid_create(arg+4, NULL);
}
else if (strstr(arg, "tag:") == arg)
{
@@ -3304,10 +3367,7 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
}
else
{
- struct dhcp_netid *newtag = opt_malloc(sizeof(struct dhcp_netid));
- newtag->net = opt_malloc(len - 3);
- strcpy(newtag->net, arg+4);
- unhide_metas(newtag->net);
+ struct dhcp_netid *newtag = dhcp_netid_create(arg+4, NULL);
if (strstr(arg, "set:") == arg)
{
@@ -3324,7 +3384,7 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
else
{
new->set = NULL;
- free(newtag);
+ dhcp_netid_free(newtag);
break;
}
}
@@ -3380,20 +3440,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
case 'M': /* --dhcp-boot */
{
- struct dhcp_netid *id = NULL;
- while (is_tag_prefix(arg))
- {
- struct dhcp_netid *newid = opt_malloc(sizeof(struct dhcp_netid));
- newid->next = id;
- id = newid;
- comma = split(arg);
- newid->net = opt_string_alloc(arg+4);
- arg = comma;
- };
+ struct dhcp_netid *id = dhcp_tags(&arg);
- if (!arg)
+ if (!id)
{
- dhcp_netid_free(id);
ret_err(gen_err);
}
else
@@ -3441,20 +3491,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
case LOPT_REPLY_DELAY: /* --dhcp-reply-delay */
{
- struct dhcp_netid *id = NULL;
- while (is_tag_prefix(arg))
- {
- struct dhcp_netid *newid = opt_malloc(sizeof(struct dhcp_netid));
- newid->next = id;
- id = newid;
- comma = split(arg);
- newid->net = opt_string_alloc(arg+4);
- arg = comma;
- };
+ struct dhcp_netid *id = dhcp_tags(&arg);
- if (!arg)
+ if (!id)
{
- dhcp_netid_free(id);
ret_err(gen_err);
}
else
@@ -3481,21 +3521,12 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
new->netid = NULL;
new->opt = 10; /* PXE_MENU_PROMPT */
-
- while (is_tag_prefix(arg))
- {
- struct dhcp_netid *nn = opt_malloc(sizeof (struct dhcp_netid));
- comma = split(arg);
- nn->next = new->netid;
- new->netid = nn;
- nn->net = opt_string_alloc(arg+4);
- arg = comma;
- }
+ new->netid = dhcp_tags(&arg);
- if (!arg)
+ if (!new->netid)
{
- dhcp_netid_free(new->netid);
- ret_err_free(gen_err, new);
+ dhcp_opt_free(new);
+ ret_err(gen_err);
}
else
{
@@ -3532,17 +3563,8 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
new->netid = NULL;
new->sname = NULL;
new->server.s_addr = 0;
+ new->netid = dhcp_tags(&arg);
- while (is_tag_prefix(arg))
- {
- struct dhcp_netid *nn = opt_malloc(sizeof (struct dhcp_netid));
- comma = split(arg);
- nn->next = new->netid;
- new->netid = nn;
- nn->net = opt_string_alloc(arg+4);
- arg = comma;
- }
-
if (arg && (comma = split(arg)))
{
for (i = 0; CSA[i]; i++)
@@ -3776,14 +3798,8 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
}
while (arg) {
- struct dhcp_netid *member = opt_malloc(sizeof(struct dhcp_netid));
comma = split(arg);
- member->next = list;
- list = member;
- if (is_tag_prefix(arg))
- member->net = opt_string_alloc(arg+4);
- else
- member->net = opt_string_alloc(arg);
+ list = dhcp_netid_create(is_tag_prefix(arg) ? arg+4 :arg, list);
arg = comma;
}
--
2.14.4
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
