Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 6 Jul 2018 03:48:51 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Brooks Davis <brooks@freebsd.org>
Cc:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r336002 - in head: usr.bin/netstat usr.sbin/tcpdrop
Message-ID:  <20180706031434.G2327@besplex.bde.org>
In-Reply-To: <201807051702.w65H2AXB058831@repo.freebsd.org>
References:  <201807051702.w65H2AXB058831@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 5 Jul 2018, Brooks Davis wrote:

> Log:
>  Work around lame warnings in ancient gcc on 32-bit platforms.
>
>  Fixes r335979.

These are correct warnings.

Why not fix the bug instead of rorking around it?

> Modified: head/usr.bin/netstat/inet.c
> ==============================================================================
> --- head/usr.bin/netstat/inet.c	Thu Jul  5 16:43:15 2018	(r336001)
> +++ head/usr.bin/netstat/inet.c	Thu Jul  5 17:02:10 2018	(r336002)
> @@ -159,12 +159,12 @@ sotoxsocket(struct socket *so, struct xsocket *xso)
>
> 	bzero(xso, sizeof *xso);
> 	xso->xso_len = sizeof *xso;
> -	xso->xso_so = (kvaddr_t)so;
> +	xso->xso_so = (kvaddr_t)(long)so;

This (and kvaddr_t having the same 64-bit size for all arches) are unportable.
Converting object pointers to integers is only portable if one or both of
intptr_t or uintptr_t exists and is used.  The above assumes that intptr_t
exists and is the same as long, and that there are no problems with the
sign mismatch between long and kvaddr_t.  But long != intptr_t on any
supported 32-bit arches.  It only has the same size.  The sign mismatch
probably doesn't matter, but this is unclear.

Correct code:

 	xso->xso_so = (kvaddr_t)(uintptr_t)so;

This only assumes that uintptr_t exists and is not larger than kvaddr_t.
FreeBSD and maybe POSIX require uintptr_t to exist, and kvaddr_t must be
chosen to be no smaller than uintptr_t.

The burden is larger for using these pointers represented as integers.
On all 32-bit arches, converting kvaddr_t back to a pointer reduces
its size, and only very broken compilers would not warn about that.
But it is a bug for the API to even have kernel pointers, so hopefully
this conversion is rarely done.

An application might just print these pointers, and should do this
without converting them to void so as to misprint them using %p format.
ps(1) attempts to do some compression when printing kernel addresses,
but IIRC this is quite broken especially on 64-bit arches where it is
most needed.  E.g, ps laxw -o wchan.  wchan is documented to trim
leading zeros when printing the value numerically.  It needs to trim
leading f's on at least amd64 and i386 3/1.  But wchan is almost useless
since it is rarely printed numerically, so it is the same as wchan for
all processes on freefall now.  The leading zeros in it are stripped
by printing it in %lx format after bogusly casting ki_wchan to long.
ki_wchan is still void *, so kinfo's ABI is very MD.  kinfo has lots
of other design errors like this, but its ABI has only been broken
once or twice since FreeBSD-~5.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180706031434.G2327>