svn commit: r296386 - head/lib/libc/rpc

[Pedro Giffuni]

On 03/04/16 17:32, Jilles Tjoelker wrote:
> On Fri, Mar 04, 2016 at 03:30:41PM +0000, Pedro F. Giffuni wrote:
>> Author: pfg
>> Date: Fri Mar  4 15:30:41 2016
>> New Revision: 296386
>> URL: https://svnweb.freebsd.org/changeset/base/296386
>
>> Log:
>>    Work around aliasing issues detected in modern GCC.
>
>>    Avoid casting gymnastics that lead to pointer aliasing by introducing an
>>    inline function as done in NetBSD (but without #if0'd WIP code).
>
>>    Obtained from:	NetBSD (CVS Rev. 1.24, 1.25)
>
>> Modified:
>>    head/lib/libc/rpc/clnt_vc.c
>
>> Modified: head/lib/libc/rpc/clnt_vc.c
>> ==============================================================================
>> --- head/lib/libc/rpc/clnt_vc.c	Fri Mar  4 14:23:34 2016	(r296385)
>> +++ head/lib/libc/rpc/clnt_vc.c	Fri Mar  4 15:30:41 2016	(r296386)
>> @@ -502,6 +502,20 @@ clnt_vc_abort(CLIENT *cl)
>>   {
>>   }
>>
>> +static __inline void
>> +htonlp(void *dst, const void *src, uint32_t incr)
>> +{
>> +	/* We are aligned, so we think */
>> +	*(uint32_t *)dst = htonl(*(const uint32_t *)src + incr);
>> +}
>> +
>> +static __inline void
>> +ntohlp(void *dst, const void *src)
>> +{
>> +	/* We are aligned, so we think */
>> +	*(uint32_t *)dst = htonl(*(const uint32_t *)src);
>> +}
>> +
>>   static bool_t
>>   clnt_vc_control(CLIENT *cl, u_int request, void *info)
>>   {
>> @@ -576,14 +590,12 @@ clnt_vc_control(CLIENT *cl, u_int reques
>>   		 * first element in the call structure
>>   		 * This will get the xid of the PREVIOUS call
>>   		 */
>> -		*(u_int32_t *)info =
>> -		    ntohl(*(u_int32_t *)(void *)&ct->ct_u.ct_mcalli);
>> +		ntohlp(info, &ct->ct_u.ct_mcalli);
>>   		break;
>>   	case CLSET_XID:
>>   		/* This will set the xid of the NEXT call */
>> -		*(u_int32_t *)(void *)&ct->ct_u.ct_mcalli =
>> -		    htonl(*((u_int32_t *)info) + 1);
>>   		/* increment by 1 as clnt_vc_call() decrements once */
>> +		htonlp(&ct->ct_u.ct_mcalli, info, 1);
>>   		break;
>>   	case CLGET_VERS:
>>   		/*
>> @@ -592,15 +604,11 @@ clnt_vc_control(CLIENT *cl, u_int reques
>>   		 * begining of the RPC header. MUST be changed if the
>>   		 * call_struct is changed
>>   		 */
>> -		*(u_int32_t *)info =
>> -		    ntohl(*(u_int32_t *)(void *)(ct->ct_u.ct_mcallc +
>> -		    4 * BYTES_PER_XDR_UNIT));
>> +		ntohlp(info, ct->ct_u.ct_mcallc + 4 * BYTES_PER_XDR_UNIT);
>>   		break;
>>
>>   	case CLSET_VERS:
>> -		*(u_int32_t *)(void *)(ct->ct_u.ct_mcallc +
>> -		    4 * BYTES_PER_XDR_UNIT) =
>> -		    htonl(*(u_int32_t *)info);
>> +		htonlp(ct->ct_u.ct_mcallc + 4 * BYTES_PER_XDR_UNIT, info, 0);
>>   		break;
>>
>>   	case CLGET_PROG:
>> @@ -610,15 +618,11 @@ clnt_vc_control(CLIENT *cl, u_int reques
>>   		 * begining of the RPC header. MUST be changed if the
>>   		 * call_struct is changed
>>   		 */
>> -		*(u_int32_t *)info =
>> -		    ntohl(*(u_int32_t *)(void *)(ct->ct_u.ct_mcallc +
>> -		    3 * BYTES_PER_XDR_UNIT));
>> +		ntohlp(info, ct->ct_u.ct_mcallc + 3 * BYTES_PER_XDR_UNIT);
>>   		break;
>>
>>   	case CLSET_PROG:
>> -		*(u_int32_t *)(void *)(ct->ct_u.ct_mcallc +
>> -		    3 * BYTES_PER_XDR_UNIT) =
>> -		    htonl(*(u_int32_t *)info);
>> +		htonlp(ct->ct_u.ct_mcallc + 3 * BYTES_PER_XDR_UNIT, info, 0);
>>   		break;
>>
>>   	default:
>
> This change just deceives GCC's warning logic without changing whether
> there is an aliasing violation or not (I don't think there is any when
> clnt_vc.c is compiled separately, since *ct does not have an effective
> type in that case).
>

Yes, I am trying to keep some changes in sync with NetBSD. They have 
some #ifdef 0'd code with memsets but the code is already ugly to
worry about code that apparently is not ready yet.

> To avoid casts, ct_mcalli could be changed to an array (of
> MCALL_MSG_SIZE / 4 elements) and used. This also avoids depending on the
> unspecified value of the elements 4 to 23 of ct_mcallc after storing to
> ct_mcalli.
>
> Note that C89 did not permit type punning via a union. NetBSD might care
> but we do not. C89 compilers are unlikely to be cunning enough for this
> to be a problem anyway.
>

Hmm.. so C89 is probably the reason the linux guys are also doing the 
same, or uglier, workarounds. :(.

> It may be that this code is too old for this kind of change.
>

I am just trying to put the forks in somewhat more consistent shape,
and finding surprises in the way. I will probably put the more
controversial changes in phabricator over the weekend.

Pedro.