Page MenuHomeFreeBSD

Remove some compatability with Seventh Edition UNIX realloc().

Authored by brooks on Aug 17 2019, 12:00 AM.



In Seventh Edition UNIX, the last pointer passed to free() was
guaranteed to not actually have been freed allowing memory to be
"compacted" via the following pattern:

foo = realloc(foo, newsize);

Further, Andrew Koenig reports in "C Traps and Pitfalls" that the
original realloc() implementation required this pattern.

The C standard is clear that this is Undefined Behavior. Modern
allocators don't support it and no portable code could rely on it so
remove this support.

Note: the removed implementation contains an off-by-one error and if
an item isn't found on the freelist, then twice as much memory as the
largest possible allocation will be copied.

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Aug 17 2019, 12:00 AM
imp accepted this revision.Aug 17 2019, 1:59 AM

This is good, and good commit message. V7 was released 40 years ago this past January. It flourished in the late 70s and early 80s as a platform people ported to (I'm aware of maybe a dozen such ports too 4 or 5 different architectures)... then all those ports migrated to System III or System V by maybe 1989 or so. The last time I saw this software pattern was in the mid 90s when I updated the program in question to not do that.... I doubt this code path has been execute in the last decade or two except by pedants testing V7 compatibility.

This revision is now accepted and ready to land.Aug 17 2019, 1:59 AM
pstef added a subscriber: pstef.Aug 17 2019, 6:07 AM
pstef added inline comments.
318 ↗(On Diff #60921)

Is this comment (rtld_malloc.c:318) talking about the same compacting as you mentioned in the commit message?

kib added inline comments.Aug 17 2019, 8:11 AM
257 ↗(On Diff #60921)

This var should be removed ...

273 ↗(On Diff #60921)

... and this condition as well, executing the code under it unconditionally.

288 ↗(On Diff #60921)

Note that cp is already freed at this point. If you claim that __crt_realloc() does not support/does not rely on accessibility of the block freed just before, then this copy should be done before __crt_free(cp).

brooks updated this revision to Diff 60989.Mon, Aug 19, 10:13 AM
  • Always follow the allocated path, don't free prematurely.
This revision now requires review to proceed.Mon, Aug 19, 10:13 AM
brooks marked 4 inline comments as done.Mon, Aug 19, 10:14 AM
brooks edited the summary of this revision. (Show Details)
kib accepted this revision.Mon, Aug 19, 12:18 PM

Please fix two style issues, no need to re-post the updated patch.

264 ↗(On Diff #60989)

I would express this as

if (op->ov_magic != MAGIC)
   return (NULL);
i = op->ov_index;

I considered should we change the if (... != MAGIC) to assert, esp. because the only consumers of this malloc are rtld and libthr, but decided that we do not.

271 ↗(On Diff #60989)

if (i != 0) {

This revision is now accepted and ready to land.Mon, Aug 19, 12:18 PM
This revision was automatically updated to reflect the committed changes.