Date: Thu, 20 Jun 2002 04:06:33 +0200 (CEST) From: Philipp Mergenthaler <philipp.mergenthaler@stud.uni-karlsruhe.de> To: FreeBSD-gnats-submit@FreeBSD.org Subject: kern/39556: [PATCH] lio_listio(2) is broken. Message-ID: <200206200206.g5K26Xjc001117@i609a.hadiko.de>
next in thread | raw e-mail | index | archive | help
>Number: 39556 >Category: kern >Synopsis: [PATCH] lio_listio(2) is broken. >Confidential: no >Severity: serious >Priority: low >Responsible: freebsd-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Wed Jun 19 19:10:01 PDT 2002 >Closed-Date: >Last-Modified: >Originator: Philipp Mergenthaler >Release: FreeBSD 5.0-CURRENT i386 >Organization: University of Karlsruhe, Germany >Environment: System: FreeBSD i609a.hadiko.de 5.0-CURRENT FreeBSD 5.0-CURRENT #551: Thu Jun 20 02:57:50 CEST 2002 p@i609a.hadiko.de:/usr/src/sys/i386/compile/I609 i386 The bug is in -stable, too. >Description: In May 2000, this commit: http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/kern/vfs_aio.c#rev1.74 introduced kqueue() and kevent() to the aio_* syscalls. This new code (ab)uses the field aio_lio_opcode of struct aiocb to pass a pointer (see kevent(2)). The code differentiates between two cases: - aio_lio_opcode == NULL (actually it should compare with "all bits zero") when _aio_aqueue() is called from e.g. aio_read() without using the kevent mechanism - aio_lio_opcode != NULL; then it assumes that the kevent mechanism is used and a pointer is passed in this field. However, this overlooks the case where _aio_aqueue() is called from lio_listio() in which case aio_lio_opcode can have the value LIO_READ or LIO_WRITE, both of which are != 0. Therefore the code interprets LIO_READ/WRITE as a pointer, which, of course, fails. >How-To-Repeat: Here is a demo program for the lio_listio() syscall. It takes up to 10 file names as command line arguments, reads these files using lio_listio() and prints the first 20 characters of each file. Because of the bug the result of the individual I/O operations is EINVAL. #include <fcntl.h> #include <errno.h> #include <time.h> #include <aio.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #define BUFFERSIZE 2048 #define NBUF 10 char buf[NBUF][BUFFERSIZE]; ssize_t retval; ssize_t nbytes; struct aiocb myaiocb[NBUF]; struct aiocb *myaiocbp[NBUF], *aiocbp, **haiocbp; int main(int argc, char *argv[]) { int i; if (argc < 2) return 1; if ((argc-1) > NBUF) return 1; aiocbp=myaiocb; haiocbp=myaiocbp; for (i=argc-1; i; i--) { bzero( aiocbp, sizeof (struct aiocb)); aiocbp->aio_lio_opcode = LIO_READ; aiocbp->aio_fildes = open( *(++argv), O_RDONLY); printf("file: %s\n", *argv); aiocbp->aio_offset = 0; aiocbp->aio_buf = malloc(BUFFERSIZE); aiocbp->aio_nbytes = BUFFERSIZE; aiocbp->aio_sigevent.sigev_notify = SIGEV_NONE; (*haiocbp++)=aiocbp++; } retval = lio_listio(LIO_WAIT, myaiocbp, argc-1, NULL); printf("lio_listio: %i\n", retval); if (retval) perror("lio_listio:"); aiocbp=myaiocb; for(i=0; i < (argc-1); i++) { retval = aio_error(aiocbp); switch (retval) { case 0: printf("%i completed successfully; read %i bytes:\n", i, aio_return(aiocbp)); printf(" %.20s\n", (volatile char*)aiocbp->aio_buf); break; case EINPROGRESS: printf("%i not yet completed!?\n", i); break; case -1: printf("%i ", i); fflush(stdout); perror("aio_error"); break; default: printf("%i ", i); errno=retval; perror("request's status"); break; } aiocbp++; } return 0; } >Fix: This patch tests for opcode == LIO_READ/WRITE in addition to NULL: Index: vfs_aio.c =================================================================== RCS file: /ncvs/src/sys/kern/vfs_aio.c,v retrieving revision 1.132 diff -u -r1.132 vfs_aio.c --- vfs_aio.c 31 May 2002 11:52:30 -0000 1.132 +++ vfs_aio.c 20 Jun 2002 01:02:33 -0000 @@ -1391,9 +1391,11 @@ */ struct kevent *kevp; - kevp = (struct kevent *)(uintptr_t)job->aio_lio_opcode; - if (kevp == NULL) + if (((struct kevent *)(uintptr_t)job->aio_lio_opcode == NULL) || + (job->aio_lio_opcode == LIO_READ) || + (job->aio_lio_opcode == LIO_WRITE)) goto no_kqueue; + kevp = (struct kevent *)(uintptr_t)job->aio_lio_opcode; error = copyin(kevp, &kev, sizeof(kev)); if (error) Actually, there is another bug: in the usual use of the aio_*() functions, the user bzero()s the struct aiocb. Therefore we shouldn't check for NULL, we should check for "all bits zero". (According to the C standard, NULL is not guaranteed to be represented as "all bits zero".) Maybe something like (untested:) char *cp; for(cp = (char *)&(job->aio_lio_opcode); cp < (char *)&(job->aio_lio_opcode) + sizeof(job->aio_lio_opcode); cp++) if (*cp) goto no_kqueue; But I guess in practice we can leave the comparison with NULL. >Release-Note: >Audit-Trail: >Unformatted: To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200206200206.g5K26Xjc001117>