This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Patch to further harden glibc malloc metadata against 1-byte overflows


I'll review this in three parts: correctness, performance, and nitpicks.

Correctness: I think this patch is correct.  All uses of the unlink()
macro are for the regular bins (which includes unsorted, but that's OK);
fastbins are handled separately, as are the sentinels and top chunks. 
Since next_chunk->prev_size is part of the current chunk, it's always
available to check.  This patch doesn't cover removing chunks from
fastbins, but that doesn't reduce the correctness of this patch.

Performance: I was unable to detect any significant performance
degradation due to the extra code, although obviously "more checking"
means "slower" to a certain extent.  This code is generally limited to
the slower paths through malloc anyway, and I think any performance
issues are too small to worry about.

Nitpicks: the patch doesn't apply as-is, at least in the recent tree I
tested against, as prev_size is not the current field name.  Also,
please use the prev_size() macro instead of a direct access.  I would
like to see "in unlink" in the crash text, but that would make the
message inconsistent with the other ones.  At least the message is
unique within malloc ;-)  Lastly, please make sure the trailing
backslashes all line up when using GNU-standard tabstops.

Overall, I think this patch is acceptable and should be applied, with
the nitpicks fixed, of course.  If you can't commit it yourself, please
post an updated patch (cc me) and I'll apply it for you.

Thanks!

DJ




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]