Bug 5070 - glibc-2.5: ../sysdeps/unix/sysv/linux/check_pf.c:68: make_request: Assertion fails
Summary: glibc-2.5: ../sysdeps/unix/sysv/linux/check_pf.c:68: make_request: Assertion ...
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: ports (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Ulrich Drepper
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-09-26 09:40 UTC by Tom Hunter
Modified: 2023-10-28 17:09 UTC (History)
3 users (show)

See Also:
Host: arm-9tdmi-linux-gnu
Target: arm-9tdmi-linux-gnu
Build: arm-9tdmi-linux-gnu
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Hunter 2007-09-26 09:40:26 UTC
When using getaddrinfo() in glibc-2.5 running on ARM9, I get the following:

../sysdeps/unix/sysv/linux/check_pf.c:68: make_request: Assertion `sizeof 
(req) - __builtin_offsetof (struct req, pad) == 3' failed.

The problem was introduced in Revision 1.7 
of "libc/sysdeps/unix/sysv/linux/check_pf.c". This works fine on x86 because of 
different alignment and packing rules required by the x86 architecture.

I suggest to do something like this instead:
--------------
 static int
 make_request (int fd, pid_t pid, bool *seen_ipv4, bool *seen_ipv6,
 	      struct in6addrinfo **in6ai, size_t *in6ailen)
 {
   struct req
   {
     struct nlmsghdr nlh;
     struct rtgenmsg g;
-    /* struct rtgenmsg consists of a single byte.  This means there
-       are three bytes of padding included in the REQ definition.
-       We make them explicit here.  */
-    char pad[3];
   } req;
   struct sockaddr_nl nladdr;
 
+  memset (&req, '\0', sizeof (req));
   req.nlh.nlmsg_len = sizeof (req);
   req.nlh.nlmsg_type = RTM_GETADDR;
   req.nlh.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
   req.nlh.nlmsg_pid = 0;
   req.nlh.nlmsg_seq = time (NULL);
   req.g.rtgen_family = AF_UNSPEC;
 
-  assert (sizeof (req) - offsetof (struct req, pad) == 3);
-  memset (req.pad, '\0', sizeof (req.pad));
 
   memset (&nladdr, '\0', sizeof (nladdr));
   nladdr.nl_family = AF_NETLINK;
 
   if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
 				    (struct sockaddr *) &nladdr,
 				    sizeof (nladdr))) < 0)
     return -1;
--------------

Regards

Tom Hunter
Comment 1 Ulrich Drepper 2007-10-02 04:01:19 UTC
It's working fine everywhere but this carp architectures.  I'm not going to make
the code perform worse just because of Arm.  Providing your own copy of that
file if you care.
Comment 2 Tom Hunter 2007-10-02 05:45:39 UTC
ARM is one of the architectures supported by glibc. You may not like it, but it 
is a fact.

Independently of the architecture, the padding done is not valid. You can't 
(and shouldn't) make any assumption about the alignment and associated padding 
done by the compiler for any architecture. GCC is free to change the alignment 
rules in any future versions. It seems rather silly to have the assert() which 
is meant to verify at runtime that your invalid assumption holds true.

I would also suggest that you don't use structures to format packets for 
networking. Packets for networking should be treated as byte streams to avoid 
any alignment/padding/byte-order issues. A standard way of doing this is 
something like this:

unsigned char buf[MaxPacketSize];
unsigned char *bp;
int skt;
size_t len;

...
bp = buf;
*bp++ = ...;
*bp++ = ...;
*bp++ = ...;
...
len = send(skt, buf, bp - buf, 0);
...
Comment 3 Ulrich Drepper 2007-10-02 14:20:07 UTC
No, Arm is not supported.  It might be supported in the ports but not in the
main package.
Comment 4 Tom Hunter 2007-10-03 03:12:08 UTC
This is a bona fide bug where Ulrich Drepper makes invalid assumptions about 
code generated by GCC. Could some else please review my suggested fix and 
commit the patch. Unfortunately it appears that Ulrich does not understand the 
need for portable code and writes for Linux on x86 platforms only.
Comment 5 Ulrich Drepper 2007-10-03 06:13:55 UTC
This has nothing to do with "x86 only".  All ABIs designed by people who have a
bit of understanding require no change.  Any change will negatively impact well
designed architectures for the sole benefit of this embedded crap.  But your own
version of the file in the add-on.
Comment 6 Joseph Myers 2007-10-03 12:10:49 UTC
The file already exists in ports, since:

2007-01-08  Daniel Jacobowitz  <dan@codesourcery.com>

        * sysdeps/unix/sysv/linux/arm/check_pf.c: New file.
        * sysdeps/unix/sysv/linux/arm/eabi/check_pf.c: New file.

(The issue only applies for old-ABI, EABI does not have this ABI peculiarity so
the EABI version just includes the generic one.)
Comment 7 Ivan Kalvachev 2007-10-05 23:42:28 UTC
If I understand correctly, the bug is that the arm aligns to 8 bytes, not 4.
This causes bigger align, that is not properly cleared but still would be send
over the network.

Instead of arguing how to clear the padding set by the compiler, I think it
would be much better to not send it at all. There is simple portable way to get
the size of the whole structure without padding and it is already used in the
assert().

Using offsetof(struct req,pad) instead of sizeof(req) would always give us the
correct size of the structure without the padding. It would still require at
least one byte padding (that we can ignore).


This solution is not only more portable, it is also faster.
And this is what I care about 
(I've never touched arm so far).


Here is sample patch against glibc 2.6.1, it applies without problems to current
cvs HEAD r1.12.

---------
--- check_pf.c.old      2007-04-25 19:05:18.000000000 +0300
+++ check_pf.c  2007-10-06 00:54:45.000000000 +0300
@@ -53,21 +53,18 @@ make_request (int fd, pid_t pid, bool *s
     struct rtgenmsg g;
     /* struct rtgenmsg consists of a single byte.  This means there
        are three bytes of padding included in the REQ definition.
-       We make them explicit here.  */
-    char pad[3];
+       We use pad as a mark for the size of the data we need.  */
+    char pad;
   } req;
   struct sockaddr_nl nladdr;
 
-  req.nlh.nlmsg_len = sizeof (req);
+  req.nlh.nlmsg_len = offsetof (struct req, pad);
   req.nlh.nlmsg_type = RTM_GETADDR;
   req.nlh.nlmsg_flags = NLM_F_ROOT | NLM_F_MATCH | NLM_F_REQUEST;
   req.nlh.nlmsg_pid = 0;
   req.nlh.nlmsg_seq = time (NULL);
   req.g.rtgen_family = AF_UNSPEC;
 
-  assert (sizeof (req) - offsetof (struct req, pad) == 3);
-  memset (req.pad, '\0', sizeof (req.pad));
-
   memset (&nladdr, '\0', sizeof (nladdr));
   nladdr.nl_family = AF_NETLINK;
 
@@ -94,7 +91,7 @@ make_request (int fd, pid_t pid, bool *s
 
   struct iovec iov = { buf, buf_size };
 
-  if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
+  if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, offsetof (struct req,
pad), 0,
                                    (struct sockaddr *) &nladdr,
                                    sizeof (nladdr))) < 0)
     goto out_fail;
---------

Thank you for your patience.
Comment 8 Joseph Myers 2009-03-18 12:20:33 UTC
I believe this was fixed by:

2008-04-21  Daniel Jacobowitz  <dan@codesourcery.com>

        * sysdeps/unix/sysv/linux/arm/check_pf.c: Update from generic
        version.
Comment 9 Jackie Rosen 2014-02-16 19:35:29 UTC Comment hidden (spam)