Re: [patch] Remove binc from vi(1)
Hi Martijn, On 06/22/17 21:32, Martijn van Duren wrote: > Attached a patch to remove the binc function from vi > and replace it with recallocarray. In principle, the memory handling in vi is very ugly, and getting it into a more standard form seems desirable - but likely to cause quite some work. Coordinating with nvi might also make sense. It is not obvious to me whether they would want to adopt recallocarray(3) - which would no doubt be desirable - or if not, whether we want to maintain yet another difference. > The functions effectively do the same thing since BINC_{GOTO,RET} > already do the nlen > llen comparison. I've run this without any > issues, but since recallocarray does extra checks and binc ALWAYS > allocates A LOT more than requested there might be some bugs > lurking. Indeed, the existing code is highly inconsistent. The binc() function increases the buffer size *by* the last argument, not *to* the last argument. So the last argument is misnamed: it is called "min", but it ought to be called "inc". All the same, it considers the buffer large enough if the last argument is smaller than the current buffer size. That is grossly inconsistent. You can only use the function to at least double the buffer size, not to grow it by, say, 50%. Even though binc() is never called directly, the BINC_* macro wrappers repeat the same size check. That's just duplicate code, always executed twice in a row. In some cases, sometimes through additional wrappers, the code calling BINC_* does things like BINC_GOTO(sp, lp, llen, llen + MAXIMUM(total, 64)) SIC: the "total" already includes "llen", yet it gets ADDED to llen, and then passed as an INCREMENT, which results in at least three times the required space being allocated. Savour it: there are at least three levels of wrappers, and at least two of these layers treat their argument as a size increment, but get passed the desired new size! In other cases, you have code like the following: BINC_GOTO(sp, tp->lb, tp->lb_len, tp->len + 1); Currently, contrary to appearance, that (usually, approximately) *doubles* the buffer size. With your patch, it increases the buffer size by one byte. In principle, such changes might result in performance degradation. You say you are running the patch without regressions. You are probably not running it on luna88k, so you may not see such degradation even if it exists. Worse, BINC_* is used at so many different places, often indirectly, that a case where performance degradation is relevant may simply not occur in your usage patterns. And yet worse, the consistently inconsistent buffer size increasing can quite likely hide instances of buffer size miscalculations, so your patch may possibly expose such miscalculations and result in buffer overruns and segfaults. Possibly in code paths that do not occur in your usage patterns. Yes, this is a giant, terrible mess. Reducing the amount of memory allocated in binc to better match what is really needed may quite possibly result in performance *improvements* because the functions are used for all kinds of buffers, even temporary ones and line buffers stored on lists, in which case wasting memory might possibly add up quite a bit. But i fear if we really want to do that, ALL USES need to be carefully audited, both to make sure that tiny increases repeatedly done in loops do not degrade performance, and to make sure that no buffer overruns get exposed. I'm not convinced such an audit can be done in one go, and even less that it can be *checked* in one go, to provide an OK. So if we really want to do this, we should probably start from the other end: change the dozens (or hundreds?) of places where this is used, often indirectly, to directly use recallocarray(3), one by one, auditing them for performance and security, one by one. Then, at the very end, remove the layers and layers of then unused wrappers. It is not a small task. In the current form, if feel unable to verify whether the patch is safe or not. It may well be, or it might not. Yours, Ingo
Re: [patch] Remove binc from vi(1)
No one? On 07/02/17 19:58, Martijn van Duren wrote: > Any takers? > > On 06/22/17 21:32, Martijn van Duren wrote: >> Hello tech@, >> >> Attached a patch to remove the binc function from vi and replace it with >> recallocarray. The functions effectively do the same thing since >> BINC_{GOTO,RET} already do the nlen > llen comparison. I've run >> this without any issues, but since recallocarray does extra checks and >> binc ALWAYS allocates A LOT more than requested there might be some >> bugs lurking. >> >> I haven't changed the name since the size component is always one and >> hence they don't expose a similar interface. >> >> OK? >> >> martijn@ >> >> Index: common/mem.h >> === >> RCS file: /cvs/src/usr.bin/vi/common/mem.h,v >> retrieving revision 1.9 >> diff -u -p -r1.9 mem.h >> --- common/mem.h 7 May 2016 14:03:01 - 1.9 >> +++ common/mem.h 22 Jun 2017 19:02:28 - >> @@ -14,30 +14,30 @@ >> /* Increase the size of a malloc'd buffer. Two versions, one that >> * returns, one that jumps to an error label. >> */ >> -#define BINC_GOTO(sp, lp, llen, nlen) { >> \ >> -void *L__bincp; \ >> +#define BINC_GOTO(sp, p, llen, nlen) { >> \ >> +void *tmpp; \ >> if ((nlen) > (llen)) { \ >> -if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ >> -== NULL)\ >> +if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ >> +== NULL) { \ >> +msgq((sp), M_SYSERR, NULL); \ >> +free(p);\ >> goto alloc_err; \ >> -/* \ >> - * !!! \ >> - * Possible pointer conversion. \ >> - */ \ >> -(lp) = L__bincp;\ >> +} \ >> +llen = nlen;\ >> +(p) = tmpp; \ >> } \ >> } >> -#define BINC_RET(sp, lp, llen, nlen) { >> \ >> -void *L__bincp; \ >> +#define BINC_RET(sp, p, llen, nlen) { >> \ >> +void *tmpp; \ >> if ((nlen) > (llen)) { \ >> -if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ >> -== NULL)\ >> +if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ >> +== NULL) { \ >> +msgq((sp), M_SYSERR, NULL); \ >> +free(p);\ >> return (1); \ >> -/* \ >> - * !!! \ >> - * Possible pointer conversion. \ >> - */ \ >> -(lp) = L__bincp;\ >> +} \ >> +llen = nlen;\ >> +(p) = tmpp; \ >> } \ >> } >> >> Index: common/util.c >> === >> RCS file: /cvs/src/usr.bin/vi/common/util.c,v >> retrieving revision 1.15 >> diff -u -p -r1.15 util.c >> --- common/util.c27 May 2016 09:18:11 - 1.15 >> +++ common/util.c22 Jun 2017 19:02:28 - >> @@ -24,43 +24,6 @@ >> >> #include "common.h" >> >> -#define MAXIMUM(a, b) (((a) > (b)) ? (a) : (b)) >> - >> -/* >> - * binc -- >> - * Increase the size of a buffer. >> - * >> - * PUBLIC: void *binc(SCR *, void *, size_t *, size_t); >> - */ >> -void * >> -binc(SCR *sp, void *bp, size_t *bsizep, size_t min) >> -{ >> -size_t csize; >> - >> -/* If already larger than the
Re: [patch] Remove binc from vi(1)
Any takers? On 06/22/17 21:32, Martijn van Duren wrote: > Hello tech@, > > Attached a patch to remove the binc function from vi and replace it with > recallocarray. The functions effectively do the same thing since > BINC_{GOTO,RET} already do the nlen > llen comparison. I've run > this without any issues, but since recallocarray does extra checks and > binc ALWAYS allocates A LOT more than requested there might be some > bugs lurking. > > I haven't changed the name since the size component is always one and > hence they don't expose a similar interface. > > OK? > > martijn@ > > Index: common/mem.h > === > RCS file: /cvs/src/usr.bin/vi/common/mem.h,v > retrieving revision 1.9 > diff -u -p -r1.9 mem.h > --- common/mem.h 7 May 2016 14:03:01 - 1.9 > +++ common/mem.h 22 Jun 2017 19:02:28 - > @@ -14,30 +14,30 @@ > /* Increase the size of a malloc'd buffer. Two versions, one that > * returns, one that jumps to an error label. > */ > -#define BINC_GOTO(sp, lp, llen, nlen) { > \ > - void *L__bincp; \ > +#define BINC_GOTO(sp, p, llen, nlen) { > \ > + void *tmpp; \ > if ((nlen) > (llen)) { \ > - if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ > - == NULL)\ > + if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ > + == NULL) { \ > + msgq((sp), M_SYSERR, NULL); \ > + free(p);\ > goto alloc_err; \ > - /* \ > - * !!! \ > - * Possible pointer conversion. \ > - */ \ > - (lp) = L__bincp;\ > + } \ > + llen = nlen;\ > + (p) = tmpp; \ > } \ > } > -#define BINC_RET(sp, lp, llen, nlen) { > \ > - void *L__bincp; \ > +#define BINC_RET(sp, p, llen, nlen) { > \ > + void *tmpp; \ > if ((nlen) > (llen)) { \ > - if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ > - == NULL)\ > + if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ > + == NULL) { \ > + msgq((sp), M_SYSERR, NULL); \ > + free(p);\ > return (1); \ > - /* \ > - * !!! \ > - * Possible pointer conversion. \ > - */ \ > - (lp) = L__bincp;\ > + } \ > + llen = nlen;\ > + (p) = tmpp; \ > } \ > } > > Index: common/util.c > === > RCS file: /cvs/src/usr.bin/vi/common/util.c,v > retrieving revision 1.15 > diff -u -p -r1.15 util.c > --- common/util.c 27 May 2016 09:18:11 - 1.15 > +++ common/util.c 22 Jun 2017 19:02:28 - > @@ -24,43 +24,6 @@ > > #include "common.h" > > -#define MAXIMUM(a, b)(((a) > (b)) ? (a) : (b)) > - > -/* > - * binc -- > - * Increase the size of a buffer. > - * > - * PUBLIC: void *binc(SCR *, void *, size_t *, size_t); > - */ > -void * > -binc(SCR *sp, void *bp, size_t *bsizep, size_t min) > -{ > - size_t csize; > - > - /* If already larger than the minimum, just return. */ > - if (min && *bsizep >= min) > - return (bp); > - > - csize
[patch] Remove binc from vi(1)
Hello tech@, Attached a patch to remove the binc function from vi and replace it with recallocarray. The functions effectively do the same thing since BINC_{GOTO,RET} already do the nlen > llen comparison. I've run this without any issues, but since recallocarray does extra checks and binc ALWAYS allocates A LOT more than requested there might be some bugs lurking. I haven't changed the name since the size component is always one and hence they don't expose a similar interface. OK? martijn@ Index: common/mem.h === RCS file: /cvs/src/usr.bin/vi/common/mem.h,v retrieving revision 1.9 diff -u -p -r1.9 mem.h --- common/mem.h7 May 2016 14:03:01 - 1.9 +++ common/mem.h22 Jun 2017 19:02:28 - @@ -14,30 +14,30 @@ /* Increase the size of a malloc'd buffer. Two versions, one that * returns, one that jumps to an error label. */ -#defineBINC_GOTO(sp, lp, llen, nlen) { \ - void *L__bincp; \ +#defineBINC_GOTO(sp, p, llen, nlen) { \ + void *tmpp; \ if ((nlen) > (llen)) { \ - if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ - == NULL)\ + if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ + == NULL) { \ + msgq((sp), M_SYSERR, NULL); \ + free(p);\ goto alloc_err; \ - /* \ -* !!! \ -* Possible pointer conversion. \ -*/ \ - (lp) = L__bincp;\ + } \ + llen = nlen;\ + (p) = tmpp; \ } \ } -#defineBINC_RET(sp, lp, llen, nlen) { \ - void *L__bincp; \ +#defineBINC_RET(sp, p, llen, nlen) { \ + void *tmpp; \ if ((nlen) > (llen)) { \ - if ((L__bincp = binc((sp), (lp), &(llen), (nlen))) \ - == NULL)\ + if (((tmpp) = recallocarray((p), (llen), (nlen), 1))\ + == NULL) { \ + msgq((sp), M_SYSERR, NULL); \ + free(p);\ return (1); \ - /* \ -* !!! \ -* Possible pointer conversion. \ -*/ \ - (lp) = L__bincp;\ + } \ + llen = nlen;\ + (p) = tmpp; \ } \ } Index: common/util.c === RCS file: /cvs/src/usr.bin/vi/common/util.c,v retrieving revision 1.15 diff -u -p -r1.15 util.c --- common/util.c 27 May 2016 09:18:11 - 1.15 +++ common/util.c 22 Jun 2017 19:02:28 - @@ -24,43 +24,6 @@ #include "common.h" -#define MAXIMUM(a, b) (((a) > (b)) ? (a) : (b)) - -/* - * binc -- - * Increase the size of a buffer. - * - * PUBLIC: void *binc(SCR *, void *, size_t *, size_t); - */ -void * -binc(SCR *sp, void *bp, size_t *bsizep, size_t min) -{ - size_t csize; - - /* If already larger than the minimum, just return. */ - if (min && *bsizep >= min) - return (bp); - - csize = *bsizep + MAXIMUM(min, 256); - REALLOC(sp, bp, csize); - - if (bp == NULL) { - /* -* Theoretically, realloc is supposed to