Re: [PATCH v7] stdio: implement fopencookie(3)

[Rich Felker <dalias@libc.org>]

On Thu, Nov 16, 2017 at 06:00:55AM +0000, William Pitcock wrote:
> The fopencookie(3) function allows the programmer to create a custom
> stdio implementation, using four hook functions which operate on a
> "cookie" data type.
>
> [...]
> +
> +struct fcookie {
> +	void *cookie;
> +	cookie_io_functions_t iofuncs;
> +};
> +
> +struct cookie_FILE {
> +	FILE f;
> +	struct fcookie fc;
> +	unsigned char buf[UNGET+BUFSIZ];
> +};
> +
> +static size_t cookieread(FILE *f, unsigned char *buf, size_t len)
> +{
> +	struct fcookie *fc = f->cookie;
> +	ssize_t ret = -1;
> +	size_t remain = len, readlen = 0;
> +
> +	if (!fc->iofuncs.read) goto bail;
> +
> +	ret = fc->iofuncs.read(fc->cookie, (char *) buf, len > 1 ? (len - 1) : 1);
> +	if (ret <= 0) goto bail;
> +
> +	readlen += ret;
> +	remain -= ret;
> +
> +	f->rpos = f->buf;
> +	ret = fc->iofuncs.read(fc->cookie, (char *) f->rpos, f->buf_size);
> +	if (ret <= 0) goto bail;
> +	f->rend = f->rpos + ret;
> +
> +	if (remain > 0) {
> +		if (remain > f->buf_size) remain = f->buf_size;
> +		memcpy(buf + readlen, f->rpos, remain);
> +		readlen += remain;
> +		f->rpos += remain;
> +	}
> +
> +	return readlen;
> +
> +bail:
> +	f->flags |= F_EOF ^ ((F_ERR^F_EOF) & ret);
> +	f->rpos = f->rend = f->buf;
> +	return readlen;
> +}

At least the following bugs are still present:

1. If len==1, the read func is called twice, and might block the
   second time even though the requested read of 1 byte succeeded.

2. The return value is wrong. It must be between 0 and len and
   represent the amount of data made available to the caller. Buffer
   filling is not supposed to be included in that since it's an
   internal matter, not something the caller sees.

3. A return value <=0 from buffer filling step is not cause to set the
   EOF or error indicators, since the part the caller requested
   succeeded. But see below...

Regarding issues 1 and 3, I think the right fix is to make the first
call conditional on len>1, and always use len-1. This makes it so the
second (possibly only) read call is never redundant/always part of the
caller-requested read, so that you can honor its EOF or error result.
Otherwise errors could be lost.

Note that you also need to be able to handle the case where there is
no buffer, in case setvbuf was called to disable buffering for the
stream. In that case only the first read should happen and it should
have full length len, not len-1.

Really all of this logic should be identical to what's in
__stdio_read.c, but with calls to the read backend rather than readv.

> +static size_t cookiewrite(FILE *f, const unsigned char *buf, size_t len)
> +{
> +	struct fcookie *fc = f->cookie;
> +	ssize_t ret;
> +	size_t len2 = f->wpos - f->wbase;
> +	if (!fc->iofuncs.write) return len;
> +	if (len2) {
> +		f->wpos = f->wbase;
> +		if (cookiewrite(f, f->wpos, len2) < len2) return 0;
> +	}
> +	ret = fc->iofuncs.write(fc->cookie, (const char *) buf, len);
> +	if (ret < 0) {
> +		f->wpos = f->wbase = f->wend = 0;
> +		f->flags |= F_ERR;
> +		return 0;
> +	}
> +	return ret;
> +}

I don't see any bugs here.

> +static off_t cookieseek(FILE *f, off_t off, int whence)
> +{
> +	struct fcookie *fc = f->cookie;
> +	int res;
> +	if (whence > 2) {

Needs to be 2U. Otherwise you don't catch negative.

> +		errno = EINVAL;
> +		return -1;
> +	}
> +	if (!fc->iofuncs.seek) {
> +		errno = ENOTSUP;
> +		return -1;
> +	}

Is there a reason to prefer ENOTSUP here? Generally ESPIPE is the
error for non-seekable FILE types, but maybe there's a reason
something else is preferred here.

> +FILE *fopencookie(void *cookie, const char *mode, cookie_io_functions_t iofuncs)
> +{
> +	struct cookie_FILE *f;
> +
> +	/* Check for valid initial mode character */
> +	if (!strchr("rwa", *mode)) {
> +		errno = EINVAL;
> +		return 0;
> +	}
> +
> +	/* Allocate FILE+fcookie+buffer or fail */
> +	if (!(f=malloc(sizeof *f))) return 0;
> +
> +	/* Zero-fill only the struct, not the buffer */
> +	memset(f, 0, sizeof(FILE));

Largely style, but I would prefer memset(&f->f, 0, sizeof f->f) (i.e.
not encoding a hidden assumption that f->f is first member and always
using sizeof the object pointed to by first arg rather than a type to
ensure consistency);

> +
> +	/* Impose mode restrictions */
> +	if (!strchr(mode, '+')) f->f.flags = (*mode == 'r') ? F_NOWR : F_NORD;
> +
> +	/* Set up our fcookie */
> +	f->fc.cookie = cookie;
> +	f->fc.iofuncs.read = iofuncs.read;
> +	f->fc.iofuncs.write = iofuncs.write;
> +	f->fc.iofuncs.seek = iofuncs.seek;
> +	f->fc.iofuncs.close = iofuncs.close;
> +
> +	f->f.fd = -1;
> +	f->f.cookie = &f->fc;
> +	f->f.buf = f->buf;

Must be f->buf+UNGET. Otherwise ungetc will underflow.

> +	f->f.buf_size = BUFSIZ;
> +	f->f.lbf = EOF;
> +
> +	/* enable opportunistic stdio locking */
> +	f->f.lock = 0;

This comment seems wrong. I think the line is unnecessary/redundant
since you memset, probably leftover from when you had it as -1.

Rich