Hi,
some of our internal checking tools emit errors when the code is using
outdated function with IPv4 only support. Because IPv6 build-time
support is now required, I think using instead inet_pton would be better.
The first attached patch replaces inet_addr with proper error reporting
function. I think this is without issues.
The second patch replaces inet_ntoa function with similar ntop. It
contains more changes, because more recent function does not use
internal hidden buffer but requires using external. I think
daemon->addrbuff is used for this case usually. It brings more lines of
code. It should also require more strict IP address format. Instead of
"10.0", full "10.0.0.0" is required. It might refuse some weird
configuration files. I think it is more correct.
Altough I have feeling some parts should be merged. For example
log_packet from src/rfc2131.c is quite similar to log6_packet from
src/rfc3315.c. They differ in address family handled and different
function signatures. Only IPv4 variant forwards log to UBUS, I expect
that is not intentional difference. I expect differences between
protocols should be as minimal as possible.
What do you think, do those changes make sense?
Cheers,
Petr
--
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemen...@redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
>From e930a56f184740089231f9c796a7132df03e47a5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Fri, 16 Jul 2021 23:30:27 +0200
Subject: [PATCH 2/2] Replace usage of inet_ntoa with inet_ntop
Use allocated buffer instead of static buffer. Not much useful because
IPv6 addresses cannot reach that functions, but might be step to merge
similar functions.
---
src/dhcp.c | 17 +++++++-----
src/helper.c | 12 ++++++---
src/option.c | 5 +++-
src/rfc2131.c | 71 ++++++++++++++++++++++++++++++++++-----------------
4 files changed, 72 insertions(+), 33 deletions(-)
diff --git a/src/dhcp.c b/src/dhcp.c
index 3d41a08..a50568b 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -468,8 +468,11 @@ void dhcp_packet(time_t now, int pxe_fd)
/* This can fail when, eg, iptables DROPS destination 255.255.255.255 */
if (errno != 0)
- my_syslog(MS_DHCP | LOG_WARNING, _("Error sending DHCP packet to %s: %s"),
- inet_ntoa(dest.sin_addr), strerror(errno));
+ {
+ inet_ntop(AF_INET, &dest.sin_addr, daemon->addrbuff, ADDRSTRLEN);
+ my_syslog(MS_DHCP | LOG_WARNING, _("Error sending DHCP packet to %s: %s"),
+ daemon->addrbuff, strerror(errno));
+ }
}
/* check against secondary interface addresses */
@@ -521,10 +524,11 @@ static void guess_range_netmask(struct in_addr addr, struct in_addr netmask)
!(is_same_net(addr, context->start, netmask) &&
is_same_net(addr, context->end, netmask)))
{
- strcpy(daemon->dhcp_buff, inet_ntoa(context->start));
- strcpy(daemon->dhcp_buff2, inet_ntoa(context->end));
+ inet_ntop(AF_INET, &context->start, daemon->dhcp_buff, sizeof(DHCP_BUFF_SZ));
+ inet_ntop(AF_INET, &context->end, daemon->dhcp_buff2, sizeof(DHCP_BUFF_SZ));
+ inet_ntop(AF_INET, &netmask, daemon->addrbuff, ADDRSTRLEN);
my_syslog(MS_DHCP | LOG_WARNING, _("DHCP range %s -- %s is not consistent with netmask %s"),
- daemon->dhcp_buff, daemon->dhcp_buff2, inet_ntoa(netmask));
+ daemon->dhcp_buff, daemon->dhcp_buff2, daemon->addrbuff);
}
context->netmask = netmask;
}
@@ -1097,7 +1101,8 @@ static int relay_upstream4(struct dhcp_relay *relay, struct dhcp_packet *mess,
if (option_bool(OPT_LOG_OPTS))
{
inet_ntop(AF_INET, &relay->local, daemon->addrbuff, ADDRSTRLEN);
- my_syslog(MS_DHCP | LOG_INFO, _("DHCP relay %s -> %s"), daemon->addrbuff, inet_ntoa(relay->server.addr4));
+ inet_ntop(AF_INET, &relay->server.addr4, daemon->dhcp_buff2, DHCP_BUFF_SZ);
+ my_syslog(MS_DHCP | LOG_INFO, _("DHCP relay %s -> %s"), daemon->addrbuff, daemon->dhcp_buff2);
}
/* Save this for replies */
diff --git a/src/helper.c b/src/helper.c
index d81de96..02340a0 100644
--- a/src/helper.c
+++ b/src/helper.c
@@ -432,7 +432,8 @@ int create_helper(int event_fd, int err_fd, uid_t uid, gid_t gid, long max_fd)
buf = grab_extradata_lua(buf, end, "relay_address");
else if (data.giaddr.s_addr != 0)
{
- lua_pushstring(lua, inet_ntoa(data.giaddr));
+ inet_ntop(AF_INET, &data.giaddr, daemon->addrbuff, ADDRSTRLEN);
+ lua_pushstring(lua, daemon->addrbuff);
lua_setfield(lua, -2, "relay_address");
}
@@ -610,8 +611,13 @@ int create_helper(int event_fd, int err_fd, uid_t uid, gid_t gid, long max_fd)
if (is6)
buf = grab_extradata(buf, end, "DNSMASQ_RELAY_ADDRESS", &err);
- else
- my_setenv("DNSMASQ_RELAY_ADDRESS", data.giaddr.s_addr != 0 ? inet_ntoa(data.giaddr) : NULL, &err);
+ else
+ {
+ const char *giaddr = NULL;
+ if (data.giaddr.s_addr != 0)
+ giaddr = inet_ntop(AF_INET, &data.giaddr, daemon->addrbuff, ADDRSTRLEN);
+ my_setenv("DNSMASQ_RELAY_ADDRESS", giaddr, &err);
+ }
for (i = 0; buf; i++)
{
diff --git a/src/option.c b/src/option.c
index 03feb29..eaf3438 100644
--- a/src/option.c
+++ b/src/option.c
@@ -3612,7 +3612,10 @@ static int one_opt(int option, char *arg, char *errstr, char *gen_err, int comma
for (configs = daemon->dhcp_conf; configs; configs = configs->next)
if ((configs->flags & CONFIG_ADDR) && configs->addr.s_addr == in.s_addr)
{
- sprintf(errstr, _("duplicate dhcp-host IP address %s"), inet_ntoa(in));
+ char addr[sizeof("127.127.127.127")];
+ strcpy(addr, arg); /* errstr rewrites arg. */
+ sprintf(errstr, _("duplicate dhcp-host IP address %s"),
+ addr);
return 0;
}
}
diff --git a/src/rfc2131.c b/src/rfc2131.c
index 0baa602..3135ba2 100644
--- a/src/rfc2131.c
+++ b/src/rfc2131.c
@@ -372,9 +372,22 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
if (!context)
{
- my_syslog(MS_DHCP | LOG_WARNING, _("no address range available for DHCP request %s %s"),
- subnet_addr.s_addr ? _("with subnet selector") : _("via"),
- subnet_addr.s_addr ? inet_ntoa(subnet_addr) : (mess->giaddr.s_addr ? inet_ntoa(mess->giaddr) : iface_name));
+ const char *via;
+ if (subnet_addr.s_addr)
+ {
+ via = _("with subnet selector");
+ inet_ntop(AF_INET, &subnet_addr, daemon->addrbuff, ADDRSTRLEN);
+ }
+ else
+ {
+ via = _("via");
+ if (mess->giaddr.s_addr)
+ inet_ntop(AF_INET, &mess->giaddr, daemon->addrbuff, ADDRSTRLEN);
+ else
+ safe_strncpy(daemon->addrbuff, iface_name, ADDRSTRLEN);
+ }
+ my_syslog(MS_DHCP | LOG_WARNING, _("no address range available for DHCP request %s %s"),
+ via, daemon->addrbuff);
return 0;
}
@@ -383,13 +396,19 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
struct dhcp_context *context_tmp;
for (context_tmp = context; context_tmp; context_tmp = context_tmp->current)
{
- strcpy(daemon->namebuff, inet_ntoa(context_tmp->start));
+ inet_ntop(AF_INET, &context_tmp->start, daemon->namebuff, MAXDNAME);
if (context_tmp->flags & (CONTEXT_STATIC | CONTEXT_PROXY))
- my_syslog(MS_DHCP | LOG_INFO, _("%u available DHCP subnet: %s/%s"),
- ntohl(mess->xid), daemon->namebuff, inet_ntoa(context_tmp->netmask));
+ {
+ inet_ntop(AF_INET, &context_tmp->netmask, daemon->addrbuff, ADDRSTRLEN);
+ my_syslog(MS_DHCP | LOG_INFO, _("%u available DHCP subnet: %s/%s"),
+ ntohl(mess->xid), daemon->namebuff, daemon->addrbuff);
+ }
else
- my_syslog(MS_DHCP | LOG_INFO, _("%u available DHCP range: %s -- %s"),
- ntohl(mess->xid), daemon->namebuff, inet_ntoa(context_tmp->end));
+ {
+ inet_ntop(AF_INET, &context_tmp->end, daemon->addrbuff, ADDRSTRLEN);
+ my_syslog(MS_DHCP | LOG_INFO, _("%u available DHCP range: %s -- %s"),
+ ntohl(mess->xid), daemon->namebuff, daemon->addrbuff);
+ }
}
}
@@ -1031,8 +1050,9 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
config->addr.s_addr == option_addr(opt).s_addr)
{
prettyprint_time(daemon->dhcp_buff, DECLINE_BACKOFF);
+ inet_ntop(AF_INET, &config->addr, daemon->addrbuff, ADDRSTRLEN);
my_syslog(MS_DHCP | LOG_WARNING, _("disabling DHCP static address %s for %s"),
- inet_ntoa(config->addr), daemon->dhcp_buff);
+ daemon->addrbuff, daemon->dhcp_buff);
config->flags |= CONFIG_DECLINED;
config->decline_time = now;
}
@@ -1078,7 +1098,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
if (have_config(config, CONFIG_ADDR))
{
- char *addrs = inet_ntoa(config->addr);
+ inet_ntop(AF_INET, &config->addr, daemon->addrbuff, ADDRSTRLEN);
if ((ltmp = lease_find_by_addr(config->addr)) &&
ltmp != lease &&
@@ -1088,7 +1108,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
unsigned char *mac = extended_hwaddr(ltmp->hwaddr_type, ltmp->hwaddr_len,
ltmp->hwaddr, ltmp->clid_len, ltmp->clid, &len);
my_syslog(MS_DHCP | LOG_WARNING, _("not using configured address %s because it is leased to %s"),
- addrs, print_mac(daemon->namebuff, mac, len));
+ daemon->addrbuff, print_mac(daemon->namebuff, mac, len));
}
else
{
@@ -1097,10 +1117,10 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
if (context->router.s_addr == config->addr.s_addr)
break;
if (tmp)
- my_syslog(MS_DHCP | LOG_WARNING, _("not using configured address %s because it is in use by the server or relay"), addrs);
+ my_syslog(MS_DHCP | LOG_WARNING, _("not using configured address %s because it is in use by the server or relay"), daemon->addrbuff);
else if (have_config(config, CONFIG_DECLINED) &&
difftime(now, config->decline_time) < (float)DECLINE_BACKOFF)
- my_syslog(MS_DHCP | LOG_WARNING, _("not using configured address %s because it was previously declined"), addrs);
+ my_syslog(MS_DHCP | LOG_WARNING, _("not using configured address %s because it was previously declined"), daemon->addrbuff);
else
conf = config->addr;
}
@@ -1303,9 +1323,10 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
a lease from one of it's MACs to give the address to another. */
if (config && config_has_mac(config, ltmp->hwaddr, ltmp->hwaddr_len, ltmp->hwaddr_type))
{
+ inet_ntop(AF_INET, <mp->addr, daemon->addrbuff, ADDRSTRLEN);
my_syslog(MS_DHCP | LOG_INFO, _("abandoning lease to %s of %s"),
print_mac(daemon->namebuff, ltmp->hwaddr, ltmp->hwaddr_len),
- inet_ntoa(ltmp->addr));
+ daemon->addrbuff);
lease = ltmp;
}
else
@@ -1674,14 +1695,15 @@ static void add_extradata_opt(struct dhcp_lease *lease, unsigned char *opt)
static void log_packet(char *type, void *addr, unsigned char *ext_mac,
int mac_len, char *interface, char *string, char *err, u32 xid)
{
- struct in_addr a;
-
if (!err && !option_bool(OPT_LOG_OPTS) && option_bool(OPT_QUIET_DHCP))
return;
- /* addr may be misaligned */
+ daemon->dhcp_buff2[0] = 0;
if (addr)
- memcpy(&a, addr, sizeof(a));
+ {
+ inet_ntop(AF_INET, addr, daemon->dhcp_buff2, DHCP_BUFF_SZ - 1);
+ //strcat(daemon->dhcp_buff2, " "); // ubus needs it without space
+ }
print_mac(daemon->namebuff, ext_mac, mac_len);
@@ -1690,7 +1712,7 @@ static void log_packet(char *type, void *addr, unsigned char *ext_mac,
ntohl(xid),
type,
interface,
- addr ? inet_ntoa(a) : "",
+ daemon->dhcp_buff2,
addr ? " " : "",
daemon->namebuff,
string ? string : "",
@@ -1699,7 +1721,7 @@ static void log_packet(char *type, void *addr, unsigned char *ext_mac,
my_syslog(MS_DHCP | LOG_INFO, "%s(%s) %s%s%s %s%s",
type,
interface,
- addr ? inet_ntoa(a) : "",
+ daemon->dhcp_buff2,
addr ? " " : "",
daemon->namebuff,
string ? string : "",
@@ -1707,9 +1729,9 @@ static void log_packet(char *type, void *addr, unsigned char *ext_mac,
#ifdef HAVE_UBUS
if (!strcmp(type, "DHCPACK"))
- ubus_event_bcast("dhcp.ack", daemon->namebuff, addr ? inet_ntoa(a) : NULL, string ? string : NULL, interface);
+ ubus_event_bcast("dhcp.ack", daemon->namebuff, addr ? daemon->dhcp_buff2 : NULL, string, interface);
else if (!strcmp(type, "DHCPRELEASE"))
- ubus_event_bcast("dhcp.release", daemon->namebuff, addr ? inet_ntoa(a) : NULL, string ? string : NULL, interface);
+ ubus_event_bcast("dhcp.release", daemon->namebuff, addr ? daemon->dhcp_buff2 : NULL, string, interface);
#endif
}
@@ -1861,7 +1883,10 @@ static size_t dhcp_packet_size(struct dhcp_packet *mess, unsigned char *agent_id
if (option_bool(OPT_LOG_OPTS))
{
if (mess->siaddr.s_addr != 0)
- my_syslog(MS_DHCP | LOG_INFO, _("%u next server: %s"), ntohl(mess->xid), inet_ntoa(mess->siaddr));
+ {
+ inet_ntop(AF_INET, &mess->siaddr, daemon->addrbuff, ADDRSTRLEN);
+ my_syslog(MS_DHCP | LOG_INFO, _("%u next server: %s"), ntohl(mess->xid), daemon->addrbuff);
+ }
if ((mess->flags & htons(0x8000)) && mess->ciaddr.s_addr == 0)
my_syslog(MS_DHCP | LOG_INFO, _("%u broadcast response"), ntohl(mess->xid));
--
2.31.1
>From 8831e60e0b0c8094fa85698911a0f722f1b1cc82 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= <pemen...@redhat.com>
Date: Thu, 15 Jul 2021 19:22:30 +0200
Subject: [PATCH 1/2] Replace inet_addr with inet_pton
inet_addr is deprecated IPv4 only function. Use inet_pton instead,
report errors is possible.
---
contrib/lease-tools/dhcp_lease_time.c | 10 +++++++---
contrib/lease-tools/dhcp_release.c | 3 +--
src/dhcp.c | 2 +-
src/network.c | 2 +-
4 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/contrib/lease-tools/dhcp_lease_time.c b/contrib/lease-tools/dhcp_lease_time.c
index 91edbfa..e2bad3c 100644
--- a/contrib/lease-tools/dhcp_lease_time.c
+++ b/contrib/lease-tools/dhcp_lease_time.c
@@ -153,7 +153,11 @@ int main(int argc, char **argv)
exit(1);
}
- lease.s_addr = inet_addr(argv[1]);
+ if (inet_pton(AF_INET, argv[1], &lease) < 1)
+ {
+ fprintf(stderr, "invalid address: %s\n", argv[1]);
+ exit(1);
+ }
memset(&packet, 0, sizeof(packet));
@@ -176,8 +180,8 @@ int main(int argc, char **argv)
*(p++) = OPTION_END;
- dest.sin_family = AF_INET;
- dest.sin_addr.s_addr = inet_addr("127.0.0.1");
+ dest.sin_family = AF_INET;
+ (void)inet_pton(AF_INET, "127.0.0.1", &dest.sin_addr);
dest.sin_port = ntohs(DHCP_SERVER_PORT);
if (sendto(fd, &packet, sizeof(packet), 0,
diff --git a/contrib/lease-tools/dhcp_release.c b/contrib/lease-tools/dhcp_release.c
index 30e77c6..c1c835b 100644
--- a/contrib/lease-tools/dhcp_release.c
+++ b/contrib/lease-tools/dhcp_release.c
@@ -288,13 +288,12 @@ int main(int argc, char **argv)
exit(1);
}
- if (inet_addr(argv[2]) == INADDR_NONE)
+ if (inet_pton(AF_INET, argv[2], &lease.s_addr) < 1)
{
perror("invalid ip address");
exit(1);
}
- lease.s_addr = inet_addr(argv[2]);
server = find_interface(lease, nl, if_nametoindex(argv[1]), fd, &ifr);
memset(&packet, 0, sizeof(packet));
diff --git a/src/dhcp.c b/src/dhcp.c
index 97324f2..3d41a08 100644
--- a/src/dhcp.c
+++ b/src/dhcp.c
@@ -922,7 +922,7 @@ void dhcp_read_ethers(void)
if (!*cp)
{
- if ((addr.s_addr = inet_addr(ip)) == (in_addr_t)-1)
+ if (inet_pton(AF_INET, ip, &addr.s_addr) < 1)
{
my_syslog(MS_DHCP | LOG_ERR, _("bad address at %s line %d"), ETHERSFILE, lineno);
continue;
diff --git a/src/network.c b/src/network.c
index 3ef71b9..3fc179d 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1688,7 +1688,7 @@ int reload_servers(char *fname)
memset(&addr, 0, sizeof(addr));
memset(&source_addr, 0, sizeof(source_addr));
- if ((addr.in.sin_addr.s_addr = inet_addr(token)) != (in_addr_t) -1)
+ if (inet_pton(AF_INET, token, &addr.in.sin_addr) > 0)
{
#ifdef HAVE_SOCKADDR_SA_LEN
source_addr.in.sin_len = addr.in.sin_len = sizeof(source_addr.in);
--
2.31.1
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss
