freebsd stable - Feb 2017 - net.inet.udp.log_in_vain strange syslog reports

On 02/02/2017 12:55, Mark Martinec wrote:
> 11.0-RELEASE-p7, net.inet.udp.log_in_vain=1
>
> The following syslog entries seem to indicate some buffer overruns
> in the reporting code (not all log lines are broken, just some).
>
> (the actual failed connection attempts were indeed there,
> it's just that the reported IP address is highly suspicious)
>
>   Mark
>
>
> Connection attempt to UDP 193.2.4.2:53 from 95.87.1521242:26375
There is no buffer overrun, so no cause for alarm.  The problem is concurrent 
usage of a single string buffer by multiple threads.  The buffer is inside 
inet_ntoa(), defined in sys/libkern/inet_ntoa.c.  In this case, it is called 
from udp_input().

Would you like to test the following patch?

Eric


diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
index 173c44c..ca2dda1 100644
--- a/sys/netinet/udp_usrreq.c
+++ b/sys/netinet/udp_usrreq.c
@@ -674,13 +674,13 @@ udp_input(struct mbuf **mp, int *offp, int proto)
                     INPLOOKUP_RLOCKPCB, ifp, m);
         if (inp == NULL) {
                 if (udp_log_in_vain) {
-                       char buf[4*sizeof "123"];
+                       char src[4*sizeof "123"];
+                       char dst[4*sizeof "123"];

-                       strcpy(buf, inet_ntoa(ip->ip_dst));
                         log(LOG_INFO,
                             "Connection attempt to UDP %s:%d from
%s:%d\n",
-                           buf, ntohs(uh->uh_dport),
inet_ntoa(ip->ip_src),
-                           ntohs(uh->uh_sport));
+                           inet_ntoa_r(ip->ip_dst, dst),
ntohs(uh->uh_dport),
+                           inet_ntoa_r(ip->ip_src, src),
ntohs(uh->uh_sport));
                 }
                 UDPSTAT_INC(udps_noport);
                 if (m->m_flags & (M_BCAST | M_MCAST)) {

> On 2017-02-02 12:55, Mark Martinec wrote:
>> 11.0-RELEASE-p7, net.inet.udp.log_in_vain=1
>> 
>> The following syslog entries seem to indicate some buffer overruns
>> in the reporting code (not all log lines are broken, just some).
>> 
>> (the actual failed connection attempts were indeed there,
>> it's just that the reported IP address is highly suspicious)
>> 
>>   Mark

2017-02-03 20:05, Eric van Gyzen wrote:
> There is no buffer overrun, so no cause for alarm.  The problem is
> concurrent usage of a single string buffer by multiple threads.  The
> buffer is inside inet_ntoa(), defined in sys/libkern/inet_ntoa.c.  In
> this case, it is called from udp_input().
> 
> Would you like to test the following patch?
> 
> Eric
> 
> 
> diff --git a/sys/netinet/udp_usrreq.c b/sys/netinet/udp_usrreq.c
> index 173c44c..ca2dda1 100644
> --- a/sys/netinet/udp_usrreq.c
> +++ b/sys/netinet/udp_usrreq.c
> @@ -674,13 +674,13 @@ udp_input(struct mbuf **mp, int *offp, int proto)
>                     INPLOOKUP_RLOCKPCB, ifp, m);
>         if (inp == NULL) {
>                 if (udp_log_in_vain) {
> -                       char buf[4*sizeof "123"];
> +                       char src[4*sizeof "123"];
> +                       char dst[4*sizeof "123"];
> 
> -                       strcpy(buf, inet_ntoa(ip->ip_dst));
>                         log(LOG_INFO,
>                             "Connection attempt to UDP %s:%d from 
> %s:%d\n",
> -                           buf, ntohs(uh->uh_dport), 
> inet_ntoa(ip->ip_src),
> -                           ntohs(uh->uh_sport));
> +                           inet_ntoa_r(ip->ip_dst, dst), 
> ntohs(uh->uh_dport),
> +                           inet_ntoa_r(ip->ip_src, src), 
> ntohs(uh->uh_sport));
>                 }
>                 UDPSTAT_INC(udps_noport);
>                 if (m->m_flags & (M_BCAST | M_MCAST)) {

Thanks, the explanation makes sense and the patch looks good (mind the 
TABs).
Running it now, expecting no surprises there.


One minor nit:
instead of a hack:

     char src[4*sizeof "123"];
     char dst[4*sizeof "123"];

it would be cleaner and in sync with the equivalent code in 
sys/netinet6/udp6_usrreq.c
to use the INET_ADDRSTRLEN constant (from sys/netinet/in.h, value 16):

     char src[INET_ADDRSTRLEN];
     char dst[INET_ADDRSTRLEN];


Hope the fix finds its way into 11.1 (or better yet, as a patch level in 
10.0).
Should I open a bug report?

   Mark