[PATCH] aio: aio_read() may not return error for invalid argument

[PATCH] aio: aio_read() may not return error for invalid argument

[Zhang Tianci]

For aio_read(0, buf, 0, -1) will return success.

Because STDIN is unseekable, so aio_read() will call read(0, buf, 0),
but read(0, buf, 0) will not return error.

So we should check that whether aio_offset is valid or not when the
file is unseekable.

Signed-off-by: Zhang Tianci <zhangtianci1@huawei.com>
---
 src/aio/aio.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/aio/aio.c b/src/aio/aio.c
index 6d34fa86..d30526e4 100644
--- a/src/aio/aio.c
+++ b/src/aio/aio.c
@@ -242,7 +242,16 @@ static void *io_thread_func(void *ctx)
 		ret = q->append ? write(fd, buf, len) : pwrite(fd, buf, len, off);
 		break;
 	case LIO_READ:
-		ret = !q->seekable ? read(fd, buf, len) : pread(fd, buf, len, off);
+		if (!q->seekable) {
+			if (off < 0) {
+				ret = -1;
+				errno = EINVAL;
+			} else {
+				ret = read(fd, buf, len);
+			}
+		} else {
+			ret = pread(fd, buf, len, off);
+		}
 		break;
 	case O_SYNC:
 		ret = fsync(fd);
@@ -253,7 +262,7 @@ static void *io_thread_func(void *ctx)
 	}
 	at.ret = ret;
 	at.err = ret<0 ? errno : 0;
-	
+
 	pthread_cleanup_pop(1);
 
 	return 0;
-- 
2.17.1

Re: [PATCH] aio: aio_read() may not return error for invalid argument

[Rich Felker]

On Sun, Oct 27, 2019 at 01:45:37PM +0800, Zhang Tianci wrote:
> For aio_read(0, buf, 0, -1) will return success.
> 
> Because STDIN is unseekable, so aio_read() will call read(0, buf, 0),
> but read(0, buf, 0) will not return error.
> 
> So we should check that whether aio_offset is valid or not when the
> file is unseekable.
> 
> Signed-off-by: Zhang Tianci <zhangtianci1@huawei.com>

I don't follow what you claim is the bug here. The error condition for
EINVAL is specified as:

    "EINVAL

    The file offset value implied by aiocbp->aio_offset would be
    invalid."

But for operations on unseekable files, no offset is implied by
aio_offset; it's not used.

At the moment I'm actually not finding the exact part of the standard
text relevant to this case. The text I was working from when I wrote
this code appears to be for write operations only:

    "If O_APPEND is not set for the file descriptor aio_fildes and if
    aio_fildes is associated with a device that is capable of seeking,
    then the requested operation takes place at the absolute position
    in the file as given by aio_offset, as if lseek() were called
    immediately prior to the operation with an offset argument equal
    to aio_offset and a whence argument equal to SEEK_SET. If O_APPEND
    is set for the file descriptor, or if aio_fildes is associated
    with a device that is incapable of seeking, write operations
    append to the file in the same order as the calls were made, with
    the following exception..."

(XSH 2.8.2) and:

    "If O_APPEND is not set for the file descriptor aio_fildes, then
    the requested operation shall take place at the absolute position
    in the file as given by aio_offset, as if lseek() were called
    immediately prior to the operation with an offset equal to
    aio_offset and a whence equal to SEEK_SET. If O_APPEND is set for
    the file descriptor, or if aio_fildes is associated with a device
    that is incapable of seeking, write operations append to the file
    in the same order as the calls were made, except under
    circumstances described in Asynchronous I/O."

(aio_write DESCRIPTION).

Indeed, aio_read seems horribly underspecified. No mention is made of
what happens if the file is not seekable or even if it is seekable but
the seek fails. It just says "as if lseek() were called immediately
prior to the operation with an offset equal to aio_offset and a whence
equal to SEEK_SET", and of course lseek can fail.

I think it's plausible that your interpretation that it should check
and fail with EINVAL on negative offset is plausible here, but I think
it's also plausible that -1 is used as a dummy value for offset with
unseekable streams, which would be useful to catch erroneous use with
a seekable stream, when the aio_read calls would not behave as
intended unless the caller tracked a running position and passed the
right offsets.

We should probably refer this to the Austin Group for an
interpretation.

Rich