* [PATCH 0/2] avoid using inet_ntoa() and inet_aton() @ 2021-06-03 9:50 Davide Caratti 2021-06-03 9:50 ` [PATCH 1/2] avoid using inet_ntoa() Davide Caratti 2021-06-03 9:50 ` [PATCH 2/2] avoid using inet_aton() Davide Caratti 0 siblings, 2 replies; 11+ messages in thread From: Davide Caratti @ 2021-06-03 9:50 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 588 bytes --] some static checkers, like rpminspect, complain about use of "forbidden" functions inet_aton() and inet_ntoa(). Use inet_pton() and inet_ntop() instead. Davide Caratti (2): avoid using inet_ntoa() avoid using inet_aton() ell/acd.c | 5 +++-- ell/dhcp-lease.c | 3 ++- ell/dhcp-server.c | 25 ++++++++++++++----------- ell/dhcp.c | 17 +++++++++++++++-- ell/rtnl.c | 7 +++++-- examples/https-server-test.c | 4 +++- 6 files changed, 42 insertions(+), 19 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] avoid using inet_ntoa() 2021-06-03 9:50 [PATCH 0/2] avoid using inet_ntoa() and inet_aton() Davide Caratti @ 2021-06-03 9:50 ` Davide Caratti 2021-06-04 16:01 ` Denis Kenzior 2021-06-03 9:50 ` [PATCH 2/2] avoid using inet_aton() Davide Caratti 1 sibling, 1 reply; 11+ messages in thread From: Davide Caratti @ 2021-06-03 9:50 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 5745 bytes --] use inet_ntop() and a temporary buffer, like it's done elsewhere for IPv6. Signed-off-by: Davide Caratti <davide.caratti@gmail.com> --- ell/acd.c | 5 +++-- ell/dhcp-lease.c | 3 ++- ell/dhcp-server.c | 11 +++++++---- ell/dhcp.c | 17 +++++++++++++++-- ell/rtnl.c | 7 +++++-- examples/https-server-test.c | 4 +++- 6 files changed, 35 insertions(+), 12 deletions(-) diff --git a/ell/acd.c b/ell/acd.c index 6989f82..e267a96 100644 --- a/ell/acd.c +++ b/ell/acd.c @@ -57,10 +57,11 @@ #define IP_STR(uint_ip) \ ({ \ + char _buf[INET_ADDRSTRLEN]; \ struct in_addr _in; \ - char *_out; \ + const char *_out; \ _in.s_addr = uint_ip; \ - _out = inet_ntoa(_in); \ + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ _out; \ }) diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c index 44c815f..94b67b4 100644 --- a/ell/dhcp-lease.c +++ b/ell/dhcp-lease.c @@ -178,12 +178,13 @@ error: static inline char *get_ip(uint32_t ip) { struct in_addr addr; + char buf[INET_ADDRSTRLEN]; if (ip == 0) return NULL; addr.s_addr = ip; - return l_strdup(inet_ntoa(addr)); + return l_strdup(inet_ntop(AF_INET, &addr, buf, INET_ADDRSTRLEN)); } LIB_EXPORT char *l_dhcp_lease_get_address(const struct l_dhcp_lease *lease) diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c index ebd4438..43f06b3 100644 --- a/ell/dhcp-server.c +++ b/ell/dhcp-server.c @@ -91,10 +91,11 @@ struct l_dhcp_server { #define IP_STR(uint_ip) \ ({ \ + char _buf[INET_ADDRSTRLEN]; \ struct in_addr _in; \ - char *_out; \ + const char *_out; \ _in.s_addr = uint_ip; \ - _out = inet_ntoa(_in); \ + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ _out; \ }) @@ -750,6 +751,7 @@ LIB_EXPORT void l_dhcp_server_destroy(struct l_dhcp_server *server) LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server) { + char buf[INET_ADDRSTRLEN]; struct in_addr ia; if (unlikely(!server)) @@ -838,9 +840,10 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server) ia.s_addr = server->address; /* In case of unit testing we don't want this to be a fatal error */ - if (!l_acd_start(server->acd, inet_ntoa(ia))) { + if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) || + !l_acd_start(server->acd, buf)) { SERVER_DEBUG("Failed to start ACD on %s, continuing without", - IP_STR(server->address)); + IP_STR(server->address)); l_acd_destroy(server->acd); server->acd = NULL; diff --git a/ell/dhcp.c b/ell/dhcp.c index a4afa95..d7809cb 100644 --- a/ell/dhcp.c +++ b/ell/dhcp.c @@ -55,6 +55,17 @@ #define BITS_PER_LONG (sizeof(unsigned long) * 8) +#define IP_STR(uint_ip) \ +({ \ + char _buf[INET_ADDRSTRLEN]; \ + struct in_addr _in; \ + const char *_out; \ + _in.s_addr = uint_ip; \ + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ + _out; \ +}) + + enum dhcp_state { DHCP_STATE_INIT, DHCP_STATE_SELECTING, @@ -778,6 +789,7 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata) struct l_dhcp_client *client = userdata; const struct dhcp_message *message = data; struct dhcp_message_iter iter; + char buf[INET_ADDRSTRLEN]; uint8_t msg_type = 0; uint8_t t, l; const void *v; @@ -911,9 +923,10 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata) ia.s_addr = client->lease->address; /* For unit testing we don't want this to be a fatal error */ - if (!l_acd_start(client->acd, inet_ntoa(ia))) { + if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) || + !l_acd_start(client->acd, buf)) { CLIENT_DEBUG("Failed to start ACD on %s, continuing", - inet_ntoa(ia)); + IP_STR(ia.s_addr)); l_acd_destroy(client->acd); client->acd = NULL; } diff --git a/ell/rtnl.c b/ell/rtnl.c index 957e749..82ad941 100644 --- a/ell/rtnl.c +++ b/ell/rtnl.c @@ -752,6 +752,7 @@ LIB_EXPORT uint32_t l_rtnl_set_powered(struct l_netlink *rtnl, int ifindex, LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, char **label, char **ip, char **broadcast) { + char buf[INET_ADDRSTRLEN]; struct in_addr in_addr; struct rtattr *attr; @@ -763,7 +764,8 @@ LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, break; in_addr = *((struct in_addr *) RTA_DATA(attr)); - *ip = l_strdup(inet_ntoa(in_addr)); + *ip = l_strdup(inet_ntop(AF_INET, &in_addr, buf, + INET_ADDRSTRLEN)); break; case IFA_BROADCAST: @@ -771,7 +773,8 @@ LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, break; in_addr = *((struct in_addr *) RTA_DATA(attr)); - *broadcast = l_strdup(inet_ntoa(in_addr)); + *broadcast = l_strdup(inet_ntop(AF_INET, &in_addr, buf, + INET_ADDRSTRLEN)); break; case IFA_LABEL: diff --git a/examples/https-server-test.c b/examples/https-server-test.c index 6362722..ddf326c 100644 --- a/examples/https-server-test.c +++ b/examples/https-server-test.c @@ -199,12 +199,14 @@ int main(int argc, char *argv[]) https_tls_ready, https_tls_disconnected, NULL); if (getenv("TLS_DEBUG")) { + char b[INET_ADDRSTRLEN]; char *str; l_tls_set_debug(tls, https_tls_debug_cb, NULL, NULL); str = l_strdup_printf("/tmp/ell-certchain-%s.pem", - inet_ntoa(client_addr.sin_addr)); + inet_ntop(AF_INET, &client_addr.sin_addr, + b, INET_ADDRSTRLEN) ?: "NA"); l_tls_set_cert_dump_path(tls, str); l_free(str); } -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-03 9:50 ` [PATCH 1/2] avoid using inet_ntoa() Davide Caratti @ 2021-06-04 16:01 ` Denis Kenzior 2021-06-07 17:00 ` d. caratti 0 siblings, 1 reply; 11+ messages in thread From: Denis Kenzior @ 2021-06-04 16:01 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 6749 bytes --] Hi Davide, On 6/3/21 4:50 AM, Davide Caratti wrote: > use inet_ntop() and a temporary buffer, like it's done elsewhere for IPv6. > > Signed-off-by: Davide Caratti <davide.caratti@gmail.com> > --- > ell/acd.c | 5 +++-- > ell/dhcp-lease.c | 3 ++- > ell/dhcp-server.c | 11 +++++++---- > ell/dhcp.c | 17 +++++++++++++++-- > ell/rtnl.c | 7 +++++-- > examples/https-server-test.c | 4 +++- Strictly speaking the examples/ change should be in its own patch. See HACKING. > 6 files changed, 35 insertions(+), 12 deletions(-) > > diff --git a/ell/acd.c b/ell/acd.c > index 6989f82..e267a96 100644 > --- a/ell/acd.c > +++ b/ell/acd.c > @@ -57,10 +57,11 @@ > > #define IP_STR(uint_ip) \ > ({ \ > + char _buf[INET_ADDRSTRLEN]; \ > struct in_addr _in; \ > - char *_out; \ > + const char *_out; \ > _in.s_addr = uint_ip; \ > - _out = inet_ntoa(_in); \ > + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ > _out; \ > }) > > diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c > index 44c815f..94b67b4 100644 > --- a/ell/dhcp-lease.c > +++ b/ell/dhcp-lease.c > @@ -178,12 +178,13 @@ error: > static inline char *get_ip(uint32_t ip) > { > struct in_addr addr; > + char buf[INET_ADDRSTRLEN]; > > if (ip == 0) > return NULL; > > addr.s_addr = ip; > - return l_strdup(inet_ntoa(addr)); > + return l_strdup(inet_ntop(AF_INET, &addr, buf, INET_ADDRSTRLEN)); > } > > LIB_EXPORT char *l_dhcp_lease_get_address(const struct l_dhcp_lease *lease) > diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c > index ebd4438..43f06b3 100644 > --- a/ell/dhcp-server.c > +++ b/ell/dhcp-server.c > @@ -91,10 +91,11 @@ struct l_dhcp_server { > > #define IP_STR(uint_ip) \ > ({ \ > + char _buf[INET_ADDRSTRLEN]; \ > struct in_addr _in; \ > - char *_out; \ > + const char *_out; \ > _in.s_addr = uint_ip; \ > - _out = inet_ntoa(_in); \ > + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ > _out; \ > }) > > @@ -750,6 +751,7 @@ LIB_EXPORT void l_dhcp_server_destroy(struct l_dhcp_server *server) > > LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server) > { > + char buf[INET_ADDRSTRLEN]; > struct in_addr ia; > > if (unlikely(!server)) > @@ -838,9 +840,10 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server) > ia.s_addr = server->address; > > /* In case of unit testing we don't want this to be a fatal error */ > - if (!l_acd_start(server->acd, inet_ntoa(ia))) { > + if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) || > + !l_acd_start(server->acd, buf)) { Note that we use only tabs for indentation... See doc/coding-style.txt for details > SERVER_DEBUG("Failed to start ACD on %s, continuing without", > - IP_STR(server->address)); > + IP_STR(server->address)); as above > > l_acd_destroy(server->acd); > server->acd = NULL; > diff --git a/ell/dhcp.c b/ell/dhcp.c > index a4afa95..d7809cb 100644 > --- a/ell/dhcp.c > +++ b/ell/dhcp.c > @@ -55,6 +55,17 @@ > > #define BITS_PER_LONG (sizeof(unsigned long) * 8) > > +#define IP_STR(uint_ip) \ > +({ \ > + char _buf[INET_ADDRSTRLEN]; \ > + struct in_addr _in; \ > + const char *_out; \ > + _in.s_addr = uint_ip; \ > + _out = inet_ntop(AF_INET, &_in, _buf, INET_ADDRSTRLEN) ?: "(inv)"; \ > + _out; \ > +}) > + > + no double empty lines please > enum dhcp_state { > DHCP_STATE_INIT, > DHCP_STATE_SELECTING, > @@ -778,6 +789,7 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata) > struct l_dhcp_client *client = userdata; > const struct dhcp_message *message = data; > struct dhcp_message_iter iter; > + char buf[INET_ADDRSTRLEN]; > uint8_t msg_type = 0; > uint8_t t, l; > const void *v; > @@ -911,9 +923,10 @@ static void dhcp_client_rx_message(const void *data, size_t len, void *userdata) > ia.s_addr = client->lease->address; > > /* For unit testing we don't want this to be a fatal error */ > - if (!l_acd_start(client->acd, inet_ntoa(ia))) { > + if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) || > + !l_acd_start(client->acd, buf)) { > CLIENT_DEBUG("Failed to start ACD on %s, continuing", > - inet_ntoa(ia)); > + IP_STR(ia.s_addr)); Not sure introducing IP_STR macro just for this is worth it, but ok... same here for indentation > l_acd_destroy(client->acd); > client->acd = NULL; > } > diff --git a/ell/rtnl.c b/ell/rtnl.c > index 957e749..82ad941 100644 > --- a/ell/rtnl.c > +++ b/ell/rtnl.c > @@ -752,6 +752,7 @@ LIB_EXPORT uint32_t l_rtnl_set_powered(struct l_netlink *rtnl, int ifindex, > LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, > char **label, char **ip, char **broadcast) > { > + char buf[INET_ADDRSTRLEN]; > struct in_addr in_addr; > struct rtattr *attr; > > @@ -763,7 +764,8 @@ LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, > break; > > in_addr = *((struct in_addr *) RTA_DATA(attr)); > - *ip = l_strdup(inet_ntoa(in_addr)); > + *ip = l_strdup(inet_ntop(AF_INET, &in_addr, buf, > + INET_ADDRSTRLEN)); > > break; > case IFA_BROADCAST: > @@ -771,7 +773,8 @@ LIB_EXPORT void l_rtnl_ifaddr4_extract(const struct ifaddrmsg *ifa, int bytes, > break; > > in_addr = *((struct in_addr *) RTA_DATA(attr)); > - *broadcast = l_strdup(inet_ntoa(in_addr)); > + *broadcast = l_strdup(inet_ntop(AF_INET, &in_addr, buf, > + INET_ADDRSTRLEN)); > > break; > case IFA_LABEL: > diff --git a/examples/https-server-test.c b/examples/https-server-test.c > index 6362722..ddf326c 100644 > --- a/examples/https-server-test.c > +++ b/examples/https-server-test.c > @@ -199,12 +199,14 @@ int main(int argc, char *argv[]) > https_tls_ready, https_tls_disconnected, NULL); > > if (getenv("TLS_DEBUG")) { > + char b[INET_ADDRSTRLEN]; > char *str; > > l_tls_set_debug(tls, https_tls_debug_cb, NULL, NULL); > > str = l_strdup_printf("/tmp/ell-certchain-%s.pem", > - inet_ntoa(client_addr.sin_addr)); > + inet_ntop(AF_INET, &client_addr.sin_addr, > + b, INET_ADDRSTRLEN) ?: "NA"); Looks like indentation problem here too. Maybe you want to put the inet_ntop business outside of l_strdup_printf call to make things more readable? > l_tls_set_cert_dump_path(tls, str); > l_free(str); > } > Regards, -Denis ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-04 16:01 ` Denis Kenzior @ 2021-06-07 17:00 ` d. caratti 2021-06-07 18:41 ` Denis Kenzior 0 siblings, 1 reply; 11+ messages in thread From: d. caratti @ 2021-06-07 17:00 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 1923 bytes --] hello Denis, thanks for reviewing! Il giorno ven 4 giu 2021 alle ore 18:01 Denis Kenzior <denkenz@gmail.com> ha scritto: > Hi Davide, > [...] > Strictly speaking the examples/ change should be in its own patch. See > HACKING. > I should have read it, as well as the coding style rules, before submitting _ sorry for not doing that. I will address coding style and remove SOB line in the v2. [...] > @@ -911,9 +923,10 @@ static void dhcp_client_rx_message(const void *data, > size_t len, void *userdata) > > ia.s_addr = client->lease->address; > > > > /* For unit testing we don't want this to be a fatal error > */ > > - if (!l_acd_start(client->acd, inet_ntoa(ia))) { > > + if (!inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) || > > + !l_acd_start(client->acd, buf)) { > > CLIENT_DEBUG("Failed to start ACD on %s, > continuing", > > - inet_ntoa(ia)); > > + IP_STR(ia.s_addr)); > > Not sure introducing IP_STR macro just for this is worth it, but ok... > that might be useful for other future CLIENT_DEBUG() users. [...] > Looks like indentation problem here too. Maybe you want to put the > inet_ntop > business outside of l_strdup_printf call to make things more readable? > yes, it seems to look better - I will also fix this. By the way, looking at the inet_ntop() documentation [1], I think that I can also avoid things like inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) ? : "(inv)"; because the first argument is hardcoded to AF_INET and also we are sure that 'buf' is INET_ADDRSTRLEN long. Under these conditions, the return value of inet_ntop() should always be 'buf'. Correct? -- davide [1] https://man7.org/linux/man-pages/man3/inet_ntop.3.html RispondiInoltra [-- Attachment #2: attachment.htm --] [-- Type: text/html, Size: 6281 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-07 17:00 ` d. caratti @ 2021-06-07 18:41 ` Denis Kenzior 2021-06-08 12:46 ` d. caratti 0 siblings, 1 reply; 11+ messages in thread From: Denis Kenzior @ 2021-06-07 18:41 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 1120 bytes --] Hi Davide, > that might be useful for other future CLIENT_DEBUG() users. > Fair enough, Wish there was a printf extension for ipv4 and ipv6 addresses like printk uses... Oh well. > > yes, it seems to look better - I will also fix this. > By the way, looking at the inet_ntop() documentation [1], I think that I can > also avoid things like > > inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN) ? : "(inv)"; > > because the first argument is hardcoded to AF_INET and also we are sure that > 'buf' is INET_ADDRSTRLEN long. > Under these conditions, the return value of inet_ntop() should always be 'buf'. > Correct? I would assume so. You can tell for sure by looking at glibc implementation for example. Oh, one other thing. Have you checked the scope rules for GCC statement expressions? inet_ntoa uses a static buffer (which glibc further enhances by marking it as thread-specific storage). So the returned pointer is guaranteed to be valid after inet_ntoa returns. The statement expression doesn't seem to do that, so that seems suspicious? Regards, -Denis ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-07 18:41 ` Denis Kenzior @ 2021-06-08 12:46 ` d. caratti 2021-06-08 16:22 ` Denis Kenzior 0 siblings, 1 reply; 11+ messages in thread From: d. caratti @ 2021-06-08 12:46 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 1783 bytes --] Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior <denkenz@gmail.com> ha scritto: > > I would assume so. You can tell for sure by looking at glibc implementation for > example. > > Oh, one other thing. Have you checked the scope rules for GCC statement > expressions? inet_ntoa uses a static buffer (which glibc further enhances by > marking it as thread-specific storage). So the returned pointer is guaranteed > to be valid after inet_ntoa returns. The statement expression doesn't seem to > do that, so that seems suspicious? ... you are right. GCC documentation [1] says: "In a statement expression, any temporaries created within a statement are destroyed at that statement’s end." so, we can't do IP_STR() that way. And using inet_ntop() + static buffer, like it's done by iproute2 [2] would actually silence the rpminspect warning I'm seeing, but not fix the actual problem: AFAIK use of a static buffer is one of the 2 reasons why inet_ntoa() has been "obsoleted". At this point, probably the cleanest thing to do is to replace IP_STR() with a proper call to inet_ntop(), preceded by declaration of buf[INET_ADDRSTRLEN] (like it's already done in some hunks of this patch). Luckily, it's just 10 places (versus 6 users of inet_ntoa(), still looks feasible). Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior <denkenz@gmail.com> ha scritto: > > > that might be useful for other future CLIENT_DEBUG() users. > > > > Fair enough, Wish there was a printf extension for ipv4 and ipv6 addresses like > printk uses... Oh well. yes, that would really make this game less boring :) [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html [2] https://github.com/shemminger/iproute2/blob/main/lib/utils.c#L980-L1020 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-08 12:46 ` d. caratti @ 2021-06-08 16:22 ` Denis Kenzior 2021-06-09 10:18 ` d. caratti 0 siblings, 1 reply; 11+ messages in thread From: Denis Kenzior @ 2021-06-08 16:22 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 2468 bytes --] Hi Davide, On 6/8/21 7:46 AM, d. caratti wrote: > Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior > <denkenz@gmail.com> ha scritto: > >> >> I would assume so. You can tell for sure by looking at glibc implementation for >> example. >> >> Oh, one other thing. Have you checked the scope rules for GCC statement >> expressions? inet_ntoa uses a static buffer (which glibc further enhances by >> marking it as thread-specific storage). So the returned pointer is guaranteed >> to be valid after inet_ntoa returns. The statement expression doesn't seem to >> do that, so that seems suspicious? > > ... you are right. GCC documentation [1] says: > > "In a statement expression, any temporaries created within a statement > are destroyed at that statement’s end." > That is what I was afraid of. > so, we can't do IP_STR() that way. And using inet_ntop() + static > buffer, like it's done by iproute2 [2] would actually > silence the rpminspect warning I'm seeing, but not fix the actual > problem: AFAIK use of a static buffer is one of the 2 reasons why > inet_ntoa() has been "obsoleted". That would be unfortunate. I'm really not sure whether this needs to be 'fixed' anyway. ell is meant for single-threaded, event driven apps. So our use of inet_ntoa is just fine. In fact we do similar stuff all over the place. > > At this point, probably the cleanest thing to do is to replace > IP_STR() with a proper call to inet_ntop(), preceded by declaration of > buf[INET_ADDRSTRLEN] (like it's already done in some hunks of this > patch). Luckily, it's just 10 places (versus 6 users of inet_ntoa(), > still looks feasible). Perhaps we can do something similar to MAC_STR instead? > > Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior > <denkenz@gmail.com> ha scritto: >> >>> that might be useful for other future CLIENT_DEBUG() users. >>> >> >> Fair enough, Wish there was a printf extension for ipv4 and ipv6 addresses like >> printk uses... Oh well. > > yes, that would really make this game less boring :) > > [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html > [2] https://github.com/shemminger/iproute2/blob/main/lib/utils.c#L980-L1020 > Yep, that's the other thing we can do. Just re-implement inet_ntoa or make our own version that covers v4/v6 with a static buffer. Not sure that is any better than just using inet_ntoa though ;) Regards, -Denis ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-08 16:22 ` Denis Kenzior @ 2021-06-09 10:18 ` d. caratti 2021-06-10 22:12 ` Denis Kenzior 0 siblings, 1 reply; 11+ messages in thread From: d. caratti @ 2021-06-09 10:18 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 4211 bytes --] Il giorno mar 8 giu 2021 alle ore 18:22 Denis Kenzior <denkenz@gmail.com> ha scritto: > > Hi Davide, > > On 6/8/21 7:46 AM, d. caratti wrote: > > Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior > > <denkenz@gmail.com> ha scritto: > > > >> > >> I would assume so. You can tell for sure by looking at glibc implementation for > >> example. this: https://sourceware.org/git/?p=glibc.git;a=blob;f=resolv/inet_ntop.c;h=c4d38c0f951013e51a4fc6eaa8a9b82e146abe5a;hb=HEAD yes, the ternary operator is not needed as long as the address family is hardcoded to AF_INET and 'buf ' fits the size of 'tmp' in inet_ntop4(). > >> Oh, one other thing. Have you checked the scope rules for GCC statement > >> expressions? inet_ntoa uses a static buffer (which glibc further enhances by > >> marking it as thread-specific storage). So the returned pointer is guaranteed > >> to be valid after inet_ntoa returns. The statement expression doesn't seem to > >> do that, so that seems suspicious? > > > > ... you are right. GCC documentation [1] says: > > > > "In a statement expression, any temporaries created within a statement > > are destroyed at that statement’s end." > > > > That is what I was afraid of. > > > so, we can't do IP_STR() that way. And using inet_ntop() + static > > buffer, like it's done by iproute2 [2] would actually > > silence the rpminspect warning I'm seeing, but not fix the actual > > problem: AFAIK use of a static buffer is one of the 2 reasons why > > inet_ntoa() has been "obsoleted". > > That would be unfortunate. I'm really not sure whether this needs to be 'fixed' > anyway. ell is meant for single-threaded, event driven apps. So our use of > inet_ntoa is just fine. In fact we do similar stuff all over the place. there's nothing wrong with using inet_ntoa() as long as the caller doesn't need to be "IPv6 friendly" and concurrency is not an issue. However, this would result in disabling the whole fedora "badfuncs" CI, that now is failing just because of inet_ntoa(). > > At this point, probably the cleanest thing to do is to replace > > IP_STR() with a proper call to inet_ntop(), preceded by declaration of > > buf[INET_ADDRSTRLEN] (like it's already done in some hunks of this > > patch). Luckily, it's just 10 places (versus 6 users of inet_ntoa(), > > still looks feasible). > > Perhaps we can do something similar to MAC_STR instead? that's similar to the NIPQUAD / NIPQUAD_FMT thing that was done in Linux before the %pI4 format specified was introduced in printk ( https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3685f25de1b0447fff381c420de ) : #include <stdio.h> #include <netinet/in.h> #include <arpa/inet.h> #define IP "%u.%u.%u.%u" #define IP_STR(uint_ip) ((unsigned char *) &uint_ip)[0], \ ((unsigned char *) &uint_ip)[1], \ ((unsigned char *) &uint_ip)[2], \ ((unsigned char *) &uint_ip)[3] int main(int argc, const char *argv[]) { struct in_addr ia = { .s_addr = 0x010200c0 }; char buf[INET_ADDRSTRLEN]; printf("test 0 %s\n", inet_ntoa(ia)); printf("test 1 %s\n", inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN)); printf("test 2 " IP "\n", IP_STR(ia.s_addr)); return 0; } > > Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior > > <denkenz@gmail.com> ha scritto: > >> > >>> that might be useful for other future CLIENT_DEBUG() users. > >>> > >> > >> Fair enough, Wish there was a printf extension for ipv4 and ipv6 addresses like > >> printk uses... Oh well. > > > > yes, that would really make this game less boring :) > > > > [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html > > [2] https://github.com/shemminger/iproute2/blob/main/lib/utils.c#L980-L1020 > > > > Yep, that's the other thing we can do. Just re-implement inet_ntoa or make our > own version that covers v4/v6 with a static buffer. Not sure that is any better > than just using inet_ntoa though ;) what about converting inet_ntoa() into inet_ntop(), and re-writing IP_STR() as above? thanks! -- davide ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] avoid using inet_ntoa() 2021-06-09 10:18 ` d. caratti @ 2021-06-10 22:12 ` Denis Kenzior 0 siblings, 0 replies; 11+ messages in thread From: Denis Kenzior @ 2021-06-10 22:12 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 3067 bytes --] Hi Davide, >> That would be unfortunate. I'm really not sure whether this needs to be 'fixed' >> anyway. ell is meant for single-threaded, event driven apps. So our use of >> inet_ntoa is just fine. In fact we do similar stuff all over the place. > > there's nothing wrong with using inet_ntoa() as long as the caller > doesn't need to be "IPv6 friendly" and concurrency is not an issue. > However, > this would result in disabling the whole fedora "badfuncs" CI, that > now is failing just because of inet_ntoa(). > Is this a worthy goal in and of itself though? What other 'badfuncs' are there? >>> At this point, probably the cleanest thing to do is to replace >>> IP_STR() with a proper call to inet_ntop(), preceded by declaration of >>> buf[INET_ADDRSTRLEN] (like it's already done in some hunks of this >>> patch). Luckily, it's just 10 places (versus 6 users of inet_ntoa(), >>> still looks feasible). >> >> Perhaps we can do something similar to MAC_STR instead? > > that's similar to the NIPQUAD / NIPQUAD_FMT thing that was done in > Linux before the %pI4 format specified was introduced in printk ( > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=3685f25de1b0447fff381c420de > ) : > > #include <stdio.h> > #include <netinet/in.h> > #include <arpa/inet.h> > > #define IP "%u.%u.%u.%u" > #define IP_STR(uint_ip) ((unsigned char *) &uint_ip)[0], \ > ((unsigned char *) &uint_ip)[1], \ > ((unsigned char *) &uint_ip)[2], \ > ((unsigned char *) &uint_ip)[3] > > int main(int argc, const char *argv[]) > { > struct in_addr ia = { .s_addr = 0x010200c0 }; > char buf[INET_ADDRSTRLEN]; > > printf("test 0 %s\n", inet_ntoa(ia)); > printf("test 1 %s\n", inet_ntop(AF_INET, &ia, buf, INET_ADDRSTRLEN)); > printf("test 2 " IP "\n", IP_STR(ia.s_addr)); > > return 0; > } > I think this or something like this would be quite reasonable to use in / for any debug messages where we print an ip. > >>> Il giorno lun 7 giu 2021 alle ore 20:41 Denis Kenzior >>> <denkenz@gmail.com> ha scritto: >>>> >>>>> that might be useful for other future CLIENT_DEBUG() users. >>>>> >>>> >>>> Fair enough, Wish there was a printf extension for ipv4 and ipv6 addresses like >>>> printk uses... Oh well. >>> >>> yes, that would really make this game less boring :) >>> >>> [1] https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html >>> [2] https://github.com/shemminger/iproute2/blob/main/lib/utils.c#L980-L1020 >>> >> >> Yep, that's the other thing we can do. Just re-implement inet_ntoa or make our >> own version that covers v4/v6 with a static buffer. Not sure that is any better >> than just using inet_ntoa though ;) > > what about converting inet_ntoa() into inet_ntop(), and re-writing > IP_STR() as above? I guess that would be OK. We just have to remember to not use inet_ntoa in the future... Regards, -Denis ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] avoid using inet_aton() 2021-06-03 9:50 [PATCH 0/2] avoid using inet_ntoa() and inet_aton() Davide Caratti 2021-06-03 9:50 ` [PATCH 1/2] avoid using inet_ntoa() Davide Caratti @ 2021-06-03 9:50 ` Davide Caratti 2021-06-04 15:41 ` Denis Kenzior 1 sibling, 1 reply; 11+ messages in thread From: Davide Caratti @ 2021-06-03 9:50 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 2302 bytes --] use inet_pton(AF_INET,...), like it's done elsewhere for IPv6. Signed-off-by: Davide Caratti <davide.caratti@gmail.com> --- ell/dhcp-server.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c index 43f06b3..e39e456 100644 --- a/ell/dhcp-server.c +++ b/ell/dhcp-server.c @@ -773,7 +773,7 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server) /* Assign a default netmask if not already */ if (!server->netmask) { - if (inet_aton("255.255.255.0", &ia) < 0) + if (inet_pton(AF_INET,"255.255.255.0", &ia) != 1) return false; server->netmask = ia.s_addr; @@ -890,12 +890,12 @@ LIB_EXPORT bool l_dhcp_server_set_ip_range(struct l_dhcp_server *server, if (unlikely(!server || !start_ip || !end_ip)) return false; - if (inet_aton(start_ip, &_host_addr) == 0) + if (inet_pton(AF_INET, start_ip, &_host_addr) != 1) return false; start = ntohl(_host_addr.s_addr); - if (inet_aton((const char *) end_ip, &_host_addr) == 0) + if (inet_pton(AF_INET, (const char *) end_ip, &_host_addr) != 1) return false; server->start_ip = start; @@ -952,7 +952,7 @@ LIB_EXPORT bool l_dhcp_server_set_ip_address(struct l_dhcp_server *server, if (unlikely(!server)) return false; - if (inet_aton(ip, &ia) < 0) + if (inet_pton(AF_INET, ip, &ia) != 1) return false; server->address = ia.s_addr; @@ -980,7 +980,7 @@ LIB_EXPORT bool l_dhcp_server_set_netmask(struct l_dhcp_server *server, if (unlikely(!server || !mask)) return false; - if (inet_aton(mask, &ia) < 0) + if (inet_pton(AF_INET, mask, &ia) != 1) return false; server->netmask = ia.s_addr; @@ -996,7 +996,7 @@ LIB_EXPORT bool l_dhcp_server_set_gateway(struct l_dhcp_server *server, if (unlikely(!server || !ip)) return false; - if (inet_aton(ip, &ia) < 0) + if (inet_pton(AF_INET, ip, &ia) != 1) return false; server->gateway = ia.s_addr; @@ -1017,7 +1017,7 @@ LIB_EXPORT bool l_dhcp_server_set_dns(struct l_dhcp_server *server, char **dns) for (i = 0; dns[i]; i++) { struct in_addr ia; - if (inet_aton(dns[i], &ia) < 0) + if (inet_pton(AF_INET, dns[i], &ia) != 1) goto failed; dns_list[i] = ia.s_addr; -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] avoid using inet_aton() 2021-06-03 9:50 ` [PATCH 2/2] avoid using inet_aton() Davide Caratti @ 2021-06-04 15:41 ` Denis Kenzior 0 siblings, 0 replies; 11+ messages in thread From: Denis Kenzior @ 2021-06-04 15:41 UTC (permalink / raw) To: ell [-- Attachment #1: Type: text/plain, Size: 389 bytes --] Hi Davide, On 6/3/21 4:50 AM, Davide Caratti wrote: > use inet_pton(AF_INET,...), like it's done elsewhere for IPv6. > > Signed-off-by: Davide Caratti <davide.caratti@gmail.com> We do not use Signed-off-by here, so I dropped this. > --- > ell/dhcp-server.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > Applied, thanks. Regards, -Denis ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-10 22:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-03 9:50 [PATCH 0/2] avoid using inet_ntoa() and inet_aton() Davide Caratti 2021-06-03 9:50 ` [PATCH 1/2] avoid using inet_ntoa() Davide Caratti 2021-06-04 16:01 ` Denis Kenzior 2021-06-07 17:00 ` d. caratti 2021-06-07 18:41 ` Denis Kenzior 2021-06-08 12:46 ` d. caratti 2021-06-08 16:22 ` Denis Kenzior 2021-06-09 10:18 ` d. caratti 2021-06-10 22:12 ` Denis Kenzior 2021-06-03 9:50 ` [PATCH 2/2] avoid using inet_aton() Davide Caratti 2021-06-04 15:41 ` Denis Kenzior
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).