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

[Szabolcs Nagy <nsz@port70.net>]

* William Pitcock <nenolod@dereferenced.org> [2017-10-05 06:48:24 +0000]:
> +FILE *fopencookie(void *cookie, const char *mode, cookie_io_functions_t iofuncs)
> +{
> +	FILE *f;
> +	struct winsize wsz;
> +	struct fcookie *fc;
> +
> +	/* 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 + sizeof *fc + UNGET + BUFSIZ))) return 0;
> +
> +	/* Zero-fill only the struct, not the buffer */
> +	memset(f, 0, sizeof *f);
> +
> +	/* Impose mode restrictions */
> +	if (!strchr(mode, '+')) f->flags = (*mode == 'r') ? F_NOWR : F_NORD;
> +
> +	/* Set up our fcookie */
> +	fc = (void *)(f + 1);

note that such malloc and pointer computation
can be problematic if *fc has larger alignment
requirement than *f

> +	fc->cookie = cookie;
> +	fc->iofuncs.read = iofuncs.read;
> +	fc->iofuncs.write = iofuncs.write;
> +	fc->iofuncs.seek = iofuncs.seek;
> +	fc->iofuncs.close = iofuncs.close;
> +
> +	f->fd = -1;
> +	f->cookie = fc;
> +	f->buf = (unsigned char *)f + sizeof *f + sizeof *fc + UNGET;
> +	f->buf_size = BUFSIZ;
> +	f->lbf = EOF;
> +
> +	/* Initialize op ptrs. No problem if some are unneeded. */
> +	f->read = cookieread;
> +	f->write = cookiewrite;
> +	f->seek = cookieseek;
> +	f->close = cookieclose;
> +
> +	if (!libc.threaded) f->lock = -1;
> +

i think this should be unconditional f->lock=0;

if a function uses user callbacks, then it cannot
be assumed that the process remains single-threaded
during the lifetime of that function call.

e.g. with your patch putc can be entered without
locking, then the user write callback may create
a thread that eventually also accesses f before
putc finishes, introducing a data race.

this shows why fopencookie hasnt been implemented
yet: there is no specification what the callbacks
may do, creating a thread is just one example
that messes up the current stdio assumptions,
recursively calling an stdio function on the same
file from a callback may cause deadlock and it's
not clear what should happen.

we can leave the corner-cases unspecified, but
then user code may start to depend on implementation
internals that we cannot change later.

(e.g. recent case in glibc: gnu make used a callback
api for glob, but assumed one of the callbacks are
not used by the implementation so it was not
initialized, now glibc started using that callback
and it took an elaborate api versioning scheme to
avoid breaking old binaries and old make code
compiled against new glibc. similar situation can
arise with fopencookie if one assumes seek is not
called in certain situations in glibc, but then
it turns out musl does call it.)

> +	/* Add new FILE to open file list */
> +	return __ofl_add(f);
> +}
> -- 
> 2.13.3