From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12108 Path: news.gmane.org!.POSTED!not-for-mail From: William Pitcock Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v7] stdio: implement fopencookie(3) Date: Fri, 17 Nov 2017 13:52:05 -0600 Message-ID: References: <20171116060055.12265-1-nenolod@dereferenced.org> <20171117173541.GD1627@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" X-Trace: blaine.gmane.org 1510948338 12639 195.159.176.226 (17 Nov 2017 19:52:18 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 17 Nov 2017 19:52:18 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-12124-gllmg-musl=m.gmane.org@lists.openwall.com Fri Nov 17 20:52:14 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 1eFmgP-00032z-Qg for gllmg-musl@m.gmane.org; Fri, 17 Nov 2017 20:52:13 +0100 Original-Received: (qmail 9257 invoked by uid 550); 17 Nov 2017 19:52:18 -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 9239 invoked from network); 17 Nov 2017 19:52:17 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dereferenced-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=i1ggoKXAImWtEO0xqVwoHwgexR6XL4INdRA6lbzXyoA=; b=bGD473CbVb1aEdhKQf3oBCZcplcJPvArteNd2w7yE17W8xbTb8DNUG/oy0oNX4ibuP ieqxatI/AUDw7ZRURmCot10GK+ZKaB7yYTQMCYkvxRsqAMM7KDFx3ZIJnh9Nr69pOoIK D0d9SaEpUKa9OXcU/Sg05mJnbr3qkPyZDCTw4XIWmHw/MCLAlXN1D4T7Nw6D7LlaJK8f 8LggDf4r2Y+M43+1jUNFabfmMWRcamYEFXANVo5u0rwchuh99gljU+Rx4lB3Jhn8iHLf GjKSOopSllZOnaIUIqZfstHT/aparPWq0zR4CaneJFXKYhU9I1fQFO8QJgJqba7VO3iT iBcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=i1ggoKXAImWtEO0xqVwoHwgexR6XL4INdRA6lbzXyoA=; b=o0RWExNm3fFQFW8rhuOJ+17AYg10kgV/D8fxoIVweatEXMyOxVB5AFd9r1cwqoI5kY rKKc9U1oiodU52Kf52dRz6g+WakI9okO+rUSUgbXwLG8b2VQVOJqzTwA2tCwcYg227C2 urR861X6cgWYMA5SJyeeYtJZLlcor7EGd/2JeRoPcNBntUDe0v6DqyQ/i1x8uQ/8WNfe WTqrC2YuyedTraRcEqWw/YGwAsagizHr1CNPoQ6OYM8y+ZVZk7PdtRCvhR9f42PT5pIY qDGi757S5VhiYVq+j0E8ITMAB3jfV6qgiD4r4Sru7F3nGeiKQ27WC6tiX2OjsDfXzGQa p3eQ== X-Gm-Message-State: AJaThX4REoBbhoq8ncpFtEsleu7gyv66WcJajvyGhldjDDeou94UCOCl /XgyEtaq0QPr/+faUmzqcjcpkNFecJGpMS4Y3iMxrs9Z X-Google-Smtp-Source: AGs4zMYa51BLgWL8srCrxsscQ0RBXWGoQ184T2+eI2sA1wRi2NrYwNQk78LM6V4W47FxjOTybrd8/FfvMHnxrTg4+P0= X-Received: by 10.200.9.43 with SMTP id t40mr10420871qth.257.1510948325564; Fri, 17 Nov 2017 11:52:05 -0800 (PST) In-Reply-To: <20171117173541.GD1627@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:12108 Archived-At: Hi, On Fri, Nov 17, 2017 at 11:35 AM, Rich Felker wrote: > 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. I believe I have reworked the read logic to match __stdio_read.c more closely. I will be sending v8 shortly, which I hope to be the final version. > >> +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. Done. >> + 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. FreeBSD's implementation[1] used ENOTSUP for this case. [1]: https://github.com/freebsd/freebsd/blob/master/lib/libc/stdio/fopencookie.c#L140-L143 glibc does not seem to set any errno at all for this case. I think it is good to follow established convention though, since the FreeBSD implementation does set errno, by matching theirs. > >> +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); Done. > >> + >> + /* 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. Done. > >> + 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. I removed it from v8. William