Re: [src] cvs commit: src/include unistd.h src/lib/libc/sys readlink.2 src/sys/compat/freebsd32 syscalls.master src/sys/kern syscalls.master vfs_syscalls.c src/sys/sys syscallsubr.h

On Fri, Feb 15, 2008 at 12:02:43PM +0200, Kostik Belousov wrote:
> On Fri, Feb 15, 2008 at 12:17:47AM +0300, Ruslan Ermilov wrote:
> > On Thu, Feb 14, 2008 at 07:38:50PM +0200, Kostik Belousov wrote:
> > > On Wed, Feb 13, 2008 at 02:35:30PM +0300, Ruslan Ermilov wrote:
> > > > [ Replying to the list. ]
> > > > 
> > > > On Tue, Feb 12, 2008 at 10:48:04PM +0200, Kostik Belousov wrote:
> > > > > > -int	 readlink(const char *, char *, int);
> > > > > > +ssize_t	 readlink(const char *, char *, size_t);
> > > > > You do understand that this changes the ABI ? size_t and int have different
> > > > > sizes on 64-bit arches, and now upper-half of the register used to pass
> > > > > the third arg is used.  Amd64, fortunately, makes very hard to load a
> > > > > non-zero into the upper half, I am not so sure about IA64/sparc64.
> > > > 
> > > > I considered that.  I've tested locally on amd64 and sparc64, and
> > > > ia64 on pluto2.freebsd.org.  Since this is only a third argument,
> > > > it's passed in a 64-bit register, and for any meaningful value of it
> > > > (0 .. INT_MAX), there's no ABI change at all.  I compared .s files.
> > > > 
> > > > : // cc -S a.c ; mv a.s a.s~ ; cc -S -DNEW a.c ; diff -u a.s~ a.s
> > > > : #include <sys/types.h>
> > > > : #include <sys/limits.h>
> > > > : 
> > > > : #ifdef NEW
> > > > : ssize_t readlink(const char *, char *, size_t);
> > > > : #else
> > > > : int readlink(const char *, char *, int);
> > > > : #endif
> > > > : 
> > > > : void
> > > > : foo(void)
> > > > : {
> > > > : 	int i;
> > > > : 	char buf[1024];
> > > > : 
> > > > : 	i = readlink("foo", buf, INT_MAX);
> > > > : }
> > > > 
> > > > > This change, IMHO, requires symbol version compat shims.
> > > > 
> > > > I don't think so.
> > > > 
> > > 
> > > The slightly contrived example below works on RELENG_7 amd64, relevant
> > > output from the truss is
> > > 	readlink("/usr/X11R6","l",1)                     = 1 (0x1)
> > > on the CURRENT gives
> > > 	readlink("/usr/X11R6","l",1)                     = -4294967295 (0xffffffff00000001)
> > > [also please note wrong output for the third readlink arg; ktrace/kdump works
> > > ok].
> > > 
> > > 	.text
> > > 	.globl	main
> > > main:	movq	$0xffffffff00000001, %rax
> > > 	movq	%rax, %rdx
> > > 	movq	$buf, %rax
> > > 	movq	%rax, %rsi
> > > 	movq	$path, %rax
> > > 	movq	%rax, %rdi
> > > 	call	readlink
> > > 	xorl	%edi, %edi
> > > 	call	exit
> > > 	
> > > 	.section	.rodata
> > > path:	.asciz	"/usr/X11R6"
> > > 
> > > 	.data
> > > 	.comm	buf, 0x80
> > 
> > This is because uio_resid is still "int".
> > 
> > : int
> > : kern_readlink(struct thread *td, char *path, enum uio_seg pathseg, char *buf,
> > :     enum uio_seg bufseg, size_t count)
> > [...]
> > :                 auio.uio_resid = count;
> > [...]
> > :         td->td_retval[0] = count - auio.uio_resid;
> > 
> > uio_resid gets the (truncated) value of "1", VOP_READLINK()
> > reads 1 char, td_retval[0] gets the value 0xffffffff00000001.
> > Any meaningful value of the third argument will work OTOH.
> > 
> The point of the conversation I started is exactly this: the domain of the
> _reasonable_ values for the third arg is changed after your commit.
> The value that was perfectly acceptable before the commit now causes
> wrong effects. I consider this to be the ABI change.

Other than crafting this with an asm or a fake prototype,
how the 64-bit value can even be passed to a syscall when
the type of the third argument is declared to be 32-bit
"int"?

In any case, the behavior isn't changed for old binaries
that expect "int" as a return type -- the returned 64-bit
value will be truncated to fit into 32-bit int, giving the
bug-to-bug compatible behavior, so there's no ABI change.
The following code, compiled statically on the old system,

: // The type of the 3rd arg is changed to allow for 64-bit.
: int readlink(const char *path, char *buf, size_t bufsiz);
: [...]
: 	int r;
: 	r = readlink("/usr/X11R6", buf, 0xffffffff00000005);

gives, on old system:

93309 a        CALL  readlink(0x41a169,0x7fffffffeb00,0xffffffff00000005)
93309 a        NAMI  "/usr/X11R6"
93309 a        RET   readlink 5

and on a new system:

28748 a        CALL  readlink(0x41a169,0x7fffffffea90,0xffffffff00000005)
28748 a        NAMI  "/usr/X11R6"
28748 a        RET   readlink 5/0xffffffff00000005

I.e., it's still "int 5".  The only change is in the
undefined upper 32 bits of the 64-bit register holding
a retval.  So, where exactly is the ABI change here?

When a similar change was made to read() and write() in 1998,
the following checks were added (to work around uio_resid
still being "int"):

: if (uap->nbyte > INT_MAX)
: 	return (EINVAL);

We can add a similar check to kern_readlink().

Longer term, we should probably also consider changing the
type of "struct uio.uio_resid" from "int" to size_t, as has
already been done by our fellow BSDs.


Cheers,
-- 
Ruslan Ermilov
ru_at_FreeBSD.org
FreeBSD committer