Subject: Patches to glibc....

From: Scott Bambrough <[email-masked]>
Date: Wed, 14 Apr 1999 17:23:18 -0400

The following patches force structures to be packed on the ARM.  The structures
ether_addr and ether_header in ethernet.h must be packed in order for tcpdump to
function correctly.  Actually tcpdump has its own version of these structures
but the principle is the same.  Similarly the structure tftphdr must be packed
or tftp and tftpd will not function correctly.

The map structure in ksc5601 has a comment that states it should be packed in
all architechures.  This is definitely not the case on the ARM.  The following 
submitted to the libc maintainers.  Ulrich is considering whether we need ARM
specific headers.  One way or another it should be fixed soon in the CVS tree.

1999-04-14 Scott Bambrough  <[email-masked]>

        * sysdeps/unix/sysv/linux/arm/net/ethernet.h: 
        struct ether_addr and struct ether_header must be packed on the ARM.
        The default alignment constraints add padding to the end of the
structures.

1999-04-14 Scott Bambrough  <[email-masked]>

        * inet/arpa/tftp.h: 
        struct tftphdr must be packed on the ARM.  The default alignment
constraints
        add padding to the end of the structure and between members.

1999-04-14 Scott Bambrough  <[email-masked]>

        * iconvdata/ksc5601.h: 
        struct map should be packed on the ARM.  The comment says the structure
should
	be packed on all architechures.  The default alignment constraints add padding
        to the end of the structure.

I found a nasty bug in sysdeps/unix/sysv/linux/arm/socket.S.  We had a bug with
ping.  It was always printing the error `Socket operation on non-socket.`.  An
strace showed the recvfrom call was failing on restart after the signal was
handled.  Its parameters were garbage.  This is because the socket call never
modifies sp; it moves sp into ip and uses ip to push the args, so it never has
to clean up the stack.

This is nice as it saves one instruction, however the syscall is not an atomic
operation.  It can be interrupted if a signal occurs.  If the signal handler
uses any stack it trashes the socket call args, because according to it the next
available stack slot is pointed at by sp.

Now for the really bad news.  mmap() suffers from the same bug.  Yuck!

Good news.  Both these patches have been applied to the CVS tree.

1999-04-14 Scott Bambrough  <[email-masked]>

        * sysdeps/unix/sysv/linux/arm/socket.S: 
        Socket calls could not be restarted after being interrupted by 
        a signal.  The parameters on the stack were corrupted by the 
        signal handler.

        * sysdeps/unix/sysv/linux/arm/mmap.S: 
        mmap calls could not be restarted after being interrupted by 
        a signal.  The parameters on the stack were corrupted by the 
        signal handler.

Scott

Index: ethernet.h
===================================================================
RCS file: /glibc/cvsfiles/libc/sysdeps/unix/sysv/linux/net/ethernet.h,v
retrieving revision 1.1
diff -r1.1 ethernet.h
36c36
< };
---
> } __attribute__((packed));
44c44
< };
---
> } __attribute__((packed)); 
Index: ksc5601.h
===================================================================
RCS file: /glibc/cvsfiles/libc/iconvdata/ksc5601.h,v
retrieving revision 1.8
diff -r1.8 ksc5601.h
35,36c35,36
<   char val[2];
< };
---
>   char val[2] __attribute__((packed));
> } __attribute__((packed));
Index: mmap.S
===================================================================
RCS file: /glibc/cvsfiles/libc/sysdeps/unix/sysv/linux/arm/mmap.S,v
retrieving revision 1.2
diff -r1.2 mmap.S
29,31c29,43
< 	mov	ip, sp
< 	stmdb	ip!, {a1-a4}
< 	mov	a1, ip
---
> 	/* This code previously moved sp into ip and stored the args using
> 	   stmdb ip!, {a1-a4}.  It did not modify sp, so the stack never had 
> 	   to be restored after the syscall completed.  It saved an 
> 	   instruction and meant no stack cleanup work was required.
> 
> 	   This will not work in the case of a mmap call being interrupted
> 	   by a signal.  If the signal handler uses any stack the arguments
> 	   to mmap will be trashed.  The results of a restart of mmap are
> 	   then unpredictable. */
> 
> 	/* store args on the stack */
> 	stmdb	sp!, {a1-a4}
> 
> 	/* do the syscall */
> 	mov	a1, sp
32a45,48
> 
> 	/* pop args off the stack. */
> 	add	sp, sp, #16
> 
Index: socket.S
===================================================================
RCS file: /glibc/cvsfiles/libc/sysdeps/unix/sysv/linux/arm/socket.S,v
retrieving revision 1.6
diff -r1.6 socket.S
38,43c38,50
< #define PUSHARGS_1	stmfd ip!, {a1}
< #define PUSHARGS_2	stmfd ip!, {a1, a2}
< #define PUSHARGS_3	stmfd ip!, {a1, a2, a3}
< #define PUSHARGS_4	stmfd ip!, {a1, a2, a3, a4}
< #define PUSHARGS_5	stmfd ip!, {a1, a2, a3, a4}	/* Caller has already pushed arg 5 */
< #define PUSHARGS_6	stmfd ip!, {a1, a2, a3, a4}
---
> #define PUSHARGS_1	stmfd sp!, {a1}
> #define PUSHARGS_2	stmfd sp!, {a1, a2}
> #define PUSHARGS_3	stmfd sp!, {a1, a2, a3}
> #define PUSHARGS_4	stmfd sp!, {a1, a2, a3, a4}
> #define PUSHARGS_5	stmfd sp!, {a1, a2, a3, a4}	/* Caller has already pushed arg 5 */
> #define PUSHARGS_6	stmfd sp!, {a1, a2, a3, a4}
> 
> #define POPARGS_1	add sp, sp, #4
> #define POPARGS_2	add sp, sp, #8
> #define POPARGS_3	add sp, sp, #12
> #define POPARGS_4	add sp, sp, #16
> #define POPARGS_5	add sp, sp, #16
> #define POPARGS_6	add sp, sp, #16 
50a58,67
> 	/* This code previously moved sp into ip and stored the args using
> 	   stmdb ip!, {a1-a4}.  It did not modify sp, so the stack never had 
> 	   to be restored after the syscall completed.  It saved an 
> 	   instruction and meant no stack cleanup work was required.
> 
> 	   This will not work in the case of a socket call being interrupted
> 	   by a signal.  If the signal handler uses any stack the arguments
> 	   to socket will be trashed.  The results of a restart of any
> 	   socket call are then unpredictable. */
> 
52d68
< 	mov ip, sp
57c73
< 	mov a2, ip
---
> 	mov a2, sp
58a75,77
> 
> 	/* Pop args off the stack */
> 	P(POPARGS_,NARGS)
Index: tftp.h
===================================================================
RCS file: /glibc/cvsfiles/libc/inet/arpa/tftp.h,v
retrieving revision 1.4
diff -r1.4 tftp.h
59,61c59,61
< 	} th_u;
< 	char	th_data[1];			/* data or error string */
< };
---
> 	} __attribute__ ((packed)) th_u;
> 	char	th_data[1] __attribute__((packed));	/* data or error string */
> } __attribute__ ((packed));

---

From: Paul Koning <[email-masked]>
Date: Wed, 14 Apr 1999 18:03:07 -0400

>>>>> "Scott" == Scott Bambrough <[email-masked]> writes:

 Scott> The following patches force structures to be packed on the
 Scott> ARM.  The structures ether_addr and ether_header in ethernet.h
 Scott> must be packed in order for tcpdump to function correctly.
 Scott> Actually tcpdump has its own version of these structures but
 Scott> the principle is the same.  Similarly the structure tftphdr
 Scott> must be packed or tftp and tftpd will not function correctly.

 Scott> The map structure in ksc5601 has a comment that states it
 Scott> should be packed in all architechures.  This is definitely not
 Scott> the case on the ARM.  The following submitted to the libc
 Scott> maintainers.  Ulrich is considering whether we need ARM
 Scott> specific headers.  One way or another it should be fixed soon
 Scott> in the CVS tree.

I assume by "...states it should be packed..." you meant that it
expects the structure to have no padding even though it was declared
without any specific attributes saying so.

If so... ARM is NOT the only platform where that assumption is false.
In fact, if you make that assumption the code is not ANSI C
compliant.  ANSI C says that structure members are aligned *at least*
to a multiple of their size.  Many platform align them exactly to that
minimum alignment, but ARM and i960, to mention two, put in additional 
padding.  The fact that most platforms use minimal padding is somewhat 
unfortunate, because it means this bug can go unnoticed for a while.

The safe way to get structures whose fields are exactly where you
planned them is to say "packed" explicitly.  That assumes, of course,
that you have a compiler that can be told such a thing.

	paul
unsubscribe: body of `unsubscribe linux-arm' to [email-masked]