Hi Benjamin, On Tue, Sep 10, 2019 at 05:52:58PM +0200, Benjamin Cama wrote: I think we should change connman_ipaddress_set_ipv4() to match connman_ipaddress_set_ipv6() in regards of the argument list. Basically, change the netmask argument to prefix len and only pass that in. And create the netmask here again. That is slightly suboptimal but I prefer that we keep the both function more alike. Wrap the two lines. They are a bit too long. calculate the prefix_length from netmaks above whenever the prefix_length is not set. Then you don't need to handle special case inse this if block. BTW, isn't the buffer too short? The string can be "128" which is 4 bytes including the \0 char. And you should also update the documentation regarding the fact that prefix length will overwrite your netmask setting. Thanks, Daniel

It is useful to catch “wrong” netmask (not CIDR), which were ignored until then. --- include/ipaddress.h | 2 +- src/config.c | 5 +++-- src/dhcp.c | 5 +++-- src/ipaddress.c | 12 ++++++++---- src/ipconfig.c | 10 ++++++---- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/include/ipaddress.h b/include/ipaddress.h index 3655ca86..a4c9871b 100644 --- a/include/ipaddress.h +++ b/include/ipaddress.h @@ -34,7 +34,7 @@ extern "C" { struct connman_ipaddress; -unsigned char connman_ipaddress_calc_netmask_len(const char *netmask); +signed char connman_ipaddress_calc_netmask_len(const char *netmask); struct connman_ipaddress *connman_ipaddress_alloc(int family); void connman_ipaddress_free(struct connman_ipaddress *ipaddress); int connman_ipaddress_set_ipv4(struct connman_ipaddress *ipaddress, diff --git a/src/config.c b/src/config.c index 62023b10..ef42d847 100644 --- a/src/config.c +++ b/src/config.c @@ -1367,9 +1367,10 @@ static int try_provision_service(struct connman_config_service *config, if (!address) return -ENOENT; - connman_ipaddress_set_ipv4(address, config->ipv4_address, + if (connman_ipaddress_set_ipv4(address, config->ipv4_address, config->ipv4_netmask, - config->ipv4_gateway); + config->ipv4_gateway)) + return -EINVAL; connman_network_set_ipv4_method(network, CONNMAN_IPCONFIG_METHOD_FIXED); diff --git a/src/dhcp.c b/src/dhcp.c index 42e9f417..09e40ce2 100644 --- a/src/dhcp.c +++ b/src/dhcp.c @@ -439,7 +439,8 @@ static void lease_available_cb(GDHCPClient *dhcp_client, gpointer user_data) enum connman_ipconfig_method old_method; char *address, *netmask = NULL, *gateway = NULL; const char *c_address, *c_gateway; - unsigned char prefixlen, c_prefixlen; + signed char prefixlen; + unsigned char c_prefixlen; bool ip_change = false; DBG("Lease available"); @@ -467,7 +468,7 @@ static void lease_available_cb(GDHCPClient *dhcp_client, gpointer user_data) gateway = g_strdup(option->data); prefixlen = connman_ipaddress_calc_netmask_len(netmask); - if (prefixlen == 255) + if (prefixlen == -1) connman_warn("netmask: %s is invalid", netmask); DBG("c_address %s", c_address); diff --git a/src/ipaddress.c b/src/ipaddress.c index ebdabbb7..c36570dd 100644 --- a/src/ipaddress.c +++ b/src/ipaddress.c @@ -33,7 +33,7 @@ #include "connman.h" -unsigned char connman_ipaddress_calc_netmask_len(const char *netmask) +signed char connman_ipaddress_calc_netmask_len(const char *netmask) { unsigned char bits; in_addr_t mask; @@ -150,9 +150,13 @@ int connman_ipaddress_set_ipv4(struct connman_ipaddress *ipaddress, ipaddress->family = AF_INET; /* check if netmask or prefixlen */ - if (g_strrstr(netmask, ".")) - ipaddress->prefixlen = connman_ipaddress_calc_netmask_len(netmask); - else + + if (g_strrstr(netmask, ".")) { + signed char prefixlen = connman_ipaddress_calc_netmask_len(netmask); + if (prefixlen == -1) + return -EINVAL; + ipaddress->prefixlen = prefixlen; + } else ipaddress->prefixlen = g_ascii_strtoull(netmask, NULL, 10); g_free(ipaddress->local); diff --git a/src/ipconfig.c b/src/ipconfig.c index b5ad1a36..8f99e111 100644 --- a/src/ipconfig.c +++ b/src/ipconfig.c @@ -2077,11 +2077,13 @@ int __connman_ipconfig_set_config(struct connman_ipconfig *ipconfig, if (prefix_length) { char prefix_length_str[3]; sprintf(prefix_length_str, "%d", prefix_length); - connman_ipaddress_set_ipv4(ipconfig->address, - address, prefix_length_str, gateway); + if (connman_ipaddress_set_ipv4(ipconfig->address, + address, prefix_length_str, gateway)) + return -EINVAL; } else - connman_ipaddress_set_ipv4(ipconfig->address, - address, netmask, gateway); + if (connman_ipaddress_set_ipv4(ipconfig->address, + address, netmask, gateway)) + return -EINVAL; } else return connman_ipaddress_set_ipv6( ipconfig->address, address, -- 2.11.0