Date: Mon, 06 Feb 2017 17:19:30 +0100 From: Mark Martinec <Mark.Martinec+freebsd@ijs.si> To: Eric van Gyzen <vangyzen@freebsd.org> Cc: freebsd-stable@freebsd.org Subject: Re: net.inet.udp.log_in_vain strange syslog reports Message-ID: <667fa3e1dd8e7cebbf4566467a7987bf@ijs.si> In-Reply-To: <ab6cbfbb-83c3-e27d-0d26-50313f171bf0@FreeBSD.org> References: <76681a24b7935674585b5ac585f4575c@ijs.si> <ab6cbfbb-83c3-e27d-0d26-50313f171bf0@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?667fa3e1dd8e7cebbf4566467a7987bf>