[Swan-dev] some warnings suppressed by pragma's (fwd)

[Paul Wouters]

Forwarded to swan-dev

---------- Forwarded message ----------
Date: Mon, 8 Jun 2015 17:16:13
From: D. Hugh Redelmeier <hugh at mimosa.com>
To: Paul Wouters <paul at nohats.ca>
Subject: Re: some warnings suppressed by pragma's

Why isn't this on the dev list?

|  From: Paul Wouters <paul at nohats.ca>

|  Attached a smal patch that Len is proposing to fix building on his
|  debian server.
| 
|  We've never done this pragma's push/pop, so I'm not sure if we
|  should do these. Looks like probably wrong band-aids.

Sadly, I think that these first two are probably the right band-aids.
The problem turns out to be way upstream.  I'd try to narrow their
application to JUST the NLMSG_OK call, at the cost of some ugliness.

I'm going to work at sending a patch to Linus.  Or maybe Lennart
should, since he worked it through.

 	 diff -urN libreswan-3.14rc1/programs/addconn/addconn.c
 	 libreswan-3.14~rc1/programs/addconn/addconn.c
 	 --- libreswan-3.14rc1/programs/addconn/addconn.c	2015-06-03
 	 14:33:52.000000000 -0400
 	 +++ libreswan-3.14~rc1/programs/addconn/addconn.c	2015-06-08
 	 11:49:58.838269454 -0400
 	 @@ -157,8 +157,11 @@

 			 /* Verify it's valid */
 			 nlhdr = (struct nlmsghdr *) buf;
 	 +#pragma GCC diagnostic push
 	 +#pragma GCC diagnostic ignored "-Wsign-compare"
 			 if (NLMSG_OK(nlhdr, readlen) == 0 ||
 	 			nlhdr->nlmsg_type == NLMSG_ERROR)
 	 +#pragma GCC diagnostic pop

NETLINK(3) documents the second arg of NLMSG_OK, called len, to be of
type (signed) int.  This isn't very sensible, but it is the contract.

Looking at NLMSG_OK and the rest of /usr/include/linux/netlink.h, I
think it is that the header is broken. BTW, broken in Fedora 21 too.
And in Linus' kernel.
<https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/netlink.h?id=refs/tags/v4.1-rc7>

Our call:
 	NLMSG_OK(nlhdr, readlen) == 0
should be:
 	!NLMSG_OK(nlhdr, readlen)
because NLMSG_OK is a test that returns a boolean result.

NLMSG_OK includes a test
 	(len) >= (int)sizeof(struct nlmsghdr)
It suggests that len should be (signed)int.

In any case, in our call, len is readlen which is of type ssize_t, a
signed version of size_t.  So it should not be the problem for this
part of the test.

NLMSG_OK also contains the conjunct:
 	(nlh)->nlmsg_len <= (len)
The left size is __u32 and the right side is ssize_t.  So len cannot
represent all possible values of type __u32.  A good reason for a
diagnostic.

So we cannot make the macro call avoid this diagnostic.

So: debian's NLMSG_OK macro has a bug.  It is obvious what the correct
fix to the header is:
 	(nlh)->nlmsg_len <= (len)
should be:
 	 (int)(nlh)->nlmsg_len <= (len)

 	 @@ -352,7 +355,10 @@
 		 /* Parse reply */
 		 struct nlmsghdr *nlmsg = (struct nlmsghdr *)msgbuf;

 	 +#pragma GCC diagnostic push
 	 +#pragma GCC diagnostic ignored "-Wsign-compare"
 	 	for (; NLMSG_OK(nlmsg, len); nlmsg = NLMSG_NEXT(nlmsg, len)) {
 	 +#pragma GCC diagnostic pop

Same problem.

 			 struct rtmsg *rtmsg;
 			 struct rtattr *rtattr;
 			 int rtlen;
 	 diff -urN libreswan-3.14rc1/programs/pluto/ikev1_spdb_struct.c
 	 libreswan-3.14~rc1/programs/pluto/ikev1_spdb_struct.c
 	 --- libreswan-3.14rc1/programs/pluto/ikev1_spdb_struct.c
 	 2015-06-03 14:33:52.000000000 -0400
 	 +++ libreswan-3.14~rc1/programs/pluto/ikev1_spdb_struct.c
 	 2015-06-08 11:29:46.509673654 -0400
 	 @@ -1623,7 +1623,7 @@
 			 pb_stream attr_pbs;
 			 enum_names *vdesc;
 			 u_int16_t ty;
 	 -		u_int32_t val;                          /* room for
 	 larger value */
 	 +		int32_t val;                          /* room for
 	 larger value */
 			 bool ipcomp_inappropriate = (proto == PROTO_IPCOMP);
 			 /* will get reset if OK */

I don't know what this comes from, but it looks like the wrong change.
Values of val are definitely unsigned.

Perhaps it comes from one of our uses of val.  Which one(s)?