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
