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

> 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

On 02/06/2017 10:19, Mark Martinec wrote:
> 
> 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];
Agreed.
> 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?
It will quite likely get into 11.1.  As for a 10.x patch, you would have
to ask re@ (I think), but I doubt it.  These messages are really just
informative and can't be used for any filtering, since the source
address could be spoofed.

Eric