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
