From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12002 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v5] stdio: implement fopencookie(3) Date: Wed, 11 Oct 2017 16:28:36 -0400 Message-ID: <20171011202836.GM1627@brightrain.aerifal.cx> References: <20171011045254.15544-1-nenolod@dereferenced.org> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1507753731 7077 195.159.176.226 (11 Oct 2017 20:28:51 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 11 Oct 2017 20:28:51 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12015-gllmg-musl=m.gmane.org@lists.openwall.com Wed Oct 11 22:28:48 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1e2NcU-00019L-MT for gllmg-musl@m.gmane.org; Wed, 11 Oct 2017 22:28:46 +0200 Original-Received: (qmail 10179 invoked by uid 550); 11 Oct 2017 20:28:49 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 10161 invoked from network); 11 Oct 2017 20:28:48 -0000 Content-Disposition: inline In-Reply-To: <20171011045254.15544-1-nenolod@dereferenced.org> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12002 Archived-At: On Wed, Oct 11, 2017 at 04:52:54AM +0000, William Pitcock wrote: > +static size_t cookieread(FILE *f, unsigned char *buf, size_t len) > +{ > + struct fcookie *fc = f->cookie; > + size_t ret, remain = len, readlen = 0; > + > + if (fc->iofuncs.read == NULL) return 0; This should probably set F_EOF (per the man page). Also as a matter of style, musl usually uses !p rather than p==0 or p==NULL, and avoids NULL entirely. I wouldn't insist on changing this if there were nothing else wrong with the code, but if you have to change it for EOF anyway it'd be nice to make it consistent with style too. > + ret = fc->iofuncs.read(fc->cookie, (char *) buf, len - 1); Except when called by fread, len is always 1, so you're making a call to iofuncs.read with a length of 0 in all these cases. I doubt iofuncs.read has accepting lenth 0 as part of its (undocumented) contract; it seems like a really bad idea to do it even if it is valid. > + if (ret == 0) goto bail_eof; And in this case it's going to have to return 0, which makes goto bail_eof clearly wrong. I don't think this code could have passed even minimal testing. Have you only tried it with fread() and not other functions like fgetc(), fgets(), etc.? This can probably be fixed by making the above code (both the iofuncs.read call and the goto check) conditional on len>1. If len>1, you are servicing an fread() that wasn't satisfied fully by existing buffer contents. Last night on IRC I suggested that it might make sense to avoid filling the buffer at all in this case (since it's a bit costly -- it requires 2 calls to iofuncs.read as you've done), but I think I was wrong. The interesting case is a number of small fread calls, each reading a length much smaller than the buffer size. In that case, if you don't fill the buffer, you end up with lots of calls to iofuncs.read. So I think you need to fill the buffer. I still do think there might be good ways to make it so iofuncs.read is always called just once, but making it work that way right now is not essential as long as it just works, and might be better done as part of a larger overhaul of how the stdio readv-like stuff works. As noted before, the current way read/write backends for stdio FILEs have to work is really klunky and designed around readv/writev for ordinary files and around the needs of fread/fwrite without much consideration for other functions. It could probably be improved considerably at some point, but doing so will be delicate and probably should be done alongside properly documenting all the internals. > + 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_eof; > + f->rend = f->rpos + ret; > + > + if (remain > f->buf_size) remain = f->buf_size; > + memcpy(buf + readlen, f->rpos, remain); > + readlen += remain; > + f->rpos += remain; > + > + return readlen; > + > +bail_eof: > + f->flags |= F_EOF; > + f->rpos = f->rend = f->buf; > + return readlen; > +} The rest of this looks like it mostly works, but seems to be missing error handling. The man page documents that the read function can return -1 on error, so I think you need to check for this and set the stream error state. > +static size_t cookiewrite(FILE *f, const unsigned char *buf, size_t len) > +{ > + struct fcookie *fc = f->cookie; > + size_t ret; > + size_t len2 = f->wpos - f->wbase; > + if (fc->iofuncs.write == NULL) return 0; > + if (len2) { > + f->wpos = f->wbase; > + if (cookiewrite(f, f->wpos, len2) < len2) return 0; > + } > + return fc->iofuncs.write(fc->cookie, (const char *) buf, len); > +} Likewise this is missing error handling. If iofuncs.write returns -1, it will wrongly return SIZE_MAX, probably blowing up pointer arithmetic in the caller. Instead it needs to return some value +static off_t cookieseek(FILE *f, off_t off, int whence) > +{ > + struct fcookie *fc = f->cookie; > + if (fc->iofuncs.seek) return fc->iofuncs.seek(fc->cookie, &off, whence); > + return -1; > +} > + > +static int cookieclose(FILE *f) > +{ > + struct fcookie *fc = f->cookie; > + if (fc->iofuncs.close) return fc->iofuncs.close(fc->cookie); > + return 0; > +} > + > +FILE *fopencookie(void *cookie, const char *mode, cookie_io_functions_t iofuncs) > +{ > + FILE *f; > + 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; In practice I think the pointer arithmetic works out okay, but it might be better to use something like: struct cookie_FILE { FILE f; struct fcookie fc; char buf[UNGET+BUFSIZ]; }; so that you don't have to rely on it being right. > + /* 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); > + 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; > + f->lock = 0; The reason f->lock is 0 and not -1 might be worth a comment. Rich