[Swan-dev] netlink.h:NLMSG_OK change to avoid GCC warnings

[Antony Antony]

Here is a data point. 

I don't see the warning you mentioned. May be I need a newer gcc? There is no warning on stock Ubuntu 15.04, i686, and libreswan master.

gcc --version
gcc (Ubuntu 4.9.2-10ubuntu13) 4.9.2

root at vivid32:~# uname -m
i686

cc -c -pthread   -g  -fexceptions -fstack-protector-all -fno-strict-aliasing -fPIE -DPIE -DFORCE_PR_ASSERT -DDNSSEC -DKLIPS -DLIBCURL -DUSE_MD5 -DHAVE_NM -DUSE_SHA2 -DUSE_SHA1 -DFIPSPRODUCTCHECK=\"/etc/system-fips\" -DIPSEC_CONF=\"/etc/ipsec.conf\" -DIPSEC_CONFDDIR=\"/etc/ipsec.d\" -DIPSEC_NSSDIR=\"/etc/ipsec.d\" -DIPSEC_CONFDIR=\"/etc\" -DIPSEC_EXECDIR=\"/usr/local/libexec/ipsec\" -DIPSEC_SBINDIR=\"/usr/local/sbin\" -DIPSEC_VARDIR=\"/var\" -DPOLICYGROUPSDIR=\"/etc/ipsec.d/policies\" -DSHARED_SECRETS_FILE=\"/etc/ipsec.secrets\" -DGCC_LINT -DALLOW_MICROSOFT_BAD_PROPOSAL -Werror -Wall -Wextra -Wformat -Wformat-nonliteral -Wformat-security -Wundef -Wmissing-declarations -Wredundant-decls -Wnested-externs -I/home/a/libreswan/ports/linux/include -I/home/a/libreswan/ports/linux/include -I/home/a/libreswan/ports/linux/include -I/home/a/libreswan/ports/linux/include -I/home/a/libreswan/programs/pluto -I/home/a/libreswan/programs/addconn/ -I/home/a/libreswan -I/home/a/libreswan/linux/include -I/home/a/libreswan/include -I/usr/include/nss -I/usr/include/nspr   /home/a/libreswan/programs/addconn/addconn.c


On Thu, Aug 27, 2015 at 03:42:26PM -0400, D. Hugh Redelmeier wrote:
> [I want to send this to the kernel API developers.  Does it look 
> reasonable?]
> 
> [Can someone test on a 32-bit system where libreswan addcon.c got build 
> warnings?  That would involve testing with and without the hacky casts to 
> unsigned short I added to the second argument of the two calls to NLMSG_OK 
> in addcon.c.  All appears well in my 64-bit system.]
> 
> Summary: please add a cast to avoid a GCC warning on 32-bit userland.
> 
> GCC seems to have added a new warning that netlink.h provokes on i386.
> netlink.h should be changed to avoid this, even though the netlink.h
> code is correct.
> 
> GCC's warning is to flag comparisons where the implicit "Usual
> Arithmetic Conversions" could lead to a surprising result.
> 
> Consider this context:
> 	int i;
> 	unsigned u;
> The C standard says that i<u is evaluated as (unsigned)i < u.
> This can easily be a surprise because a negative value of i will be 
> silently treated as a very large positive value.
> 
> In include/uapi/linux/netlink.h:
> #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
> 			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> 			   (nlh)->nlmsg_len <= (len))
> 
> This comparison looks suspicious to GCC:
> 	(nlh)->nlmsg_len <= (len)
> nlmsg_len is of type __u32.  If int is 32 bits, this compare will be
> done as
> 	(nlh)->nlmsg_len <= (unsigned)(len)
> We know that this is actually safe because the first conjunct
> determined that len isn't negative but the GCC apparently doesn't know.
> 
> A change that would calm GCC and also be correct would be to add
> a cast to:
> 	(nlh)->nlmsg_len <= (unsigned)(len))
> But I imagine that len might well actually have type ssize_t.  It is
> often the result of a call to recvfrom(2), which is a ssize_t.  So I
> think that this would be safer:
> 	(nlh)->nlmsg_len <= (size_t)(len))
> I know of no system where size_t is narrower than unsigned.
> 
> This problem came up when building a userland component of libreswan in a 
> 32-bit environment with a recent GCC and was reported by Lennart Sorensen.
> 
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index cf6a65c..0343f58 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -86,7 +86,7 @@ struct nlmsghdr {
>  				  (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
>  #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
>  			   (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> -			   (nlh)->nlmsg_len <= (len))
> +			   (nlh)->nlmsg_len <= (size_t)(len))
>  #define NLMSG_PAYLOAD(nlh,len) ((nlh)->nlmsg_len - NLMSG_SPACE((len)))
>  
>  #define NLMSG_NOOP		0x1	/* Nothing.		*/
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
>