Re: (ITS#6303) Buffer overflow with new glibc

[h.b.furuseth@usit.uio.no]

jzeleny@redhat.com writes:
>> Could you test if this works instead?
>>    http://folk.uio.no/hbf/ol-struct-hack-1.patch
>> If that doesn't work, similar code elsewhere may be in danger.
>
> This hack seems to work just fine.

Thank you.

> But I'd like to ask if it's the
> right way to do it - I mean, as you said the code isn't run that
> often, so there is no need to consider performance and I think my
> patch offers cleaner solution.

That discussion gets a bit bigger than the problem, but anyway:

I'd say the right way to write it is the way we'd be happy to se people
use it as demo/template code.  So yes, even though the back-ldif code
isn't executed much, it's good to use simple tricks like this anyway.
As long as they are not buggy or unnecessarily complicate the code.

The silly way to do it was to use a well-known but officially
dubious trick, the struct hack, with a non-standard notation: "char
fname;" instead of "char fname[1];", and macroized at that.

I see several mentions on gcc.gnu.org about breaking and un-breaking the
struct hack.  I haven't looked closely, but for now I'll guess gcc
doesn't see the original back-ldif code as the struct hack and doesn't
un-break it:

The code stores a struct bvlist object in the first part of the malloced
memory, thus informing the compiler that any padding bytes in that
bvlist object have unspecified values.  Thus I guess the compiler is
free to ignore whatever back-ldif puts in these bytes.

Reading gcc.gnu.org about the struct hack was a bit unsettling though,
so it seemed better to avoid it.  Maybe others in the project have
different opinion, I don't know.  I seem to be arguing with myself
about readability anyway:-)


As for malloc, slapd's main problem with malloc is memory fragmentation.
Performance comes second.

And for both memory fragmentation and speed, the "righter" way for small
data freed by the same thread would not be malloc() but ber_memalloc_x()
or slap_sl_malloc() with a non-NULL context parameter.  Oh well, maybe
later - if anyone cares.

-- 
Hallvard