Skip to content

Conversation

@DerDakon
Copy link
Member

@DerDakon DerDakon commented Apr 13, 2020

  • it's as simple as possible, probably intentional. This makes things like alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it reallocates is already the last thing in the batch, and simply extends that, or if alloc_free() has the last block and adds things back to the pool (ideally with zeroing for security reasons).
  • it makes it basically impossible to detect overruns on early small allocations with tools like valgrind, address sanitizer or friends as those allocations never hit malloc(), so are never traced by any custom tools. Any overrun will just go into the next variable, untracable.
  • allocators do their own buffering, alignment, and so on. If they are broken, your system is severely screwed. And if we need more money^H^H^H^Hemory we will hit that buggy allocator anyway, making things harder to detect as it will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or something like that anymore. We expect a reasonably sane OS. Everything else will get us screwed in much more severe ways.
  • it has shown issues when coming close to 4GiB allocations (fix overflow in alloc.c #37, fix possible integer overflow in alloc() #109, older reports). DJB correctly says that an allocation of that size in your mailer shows that you have already done something wrong, but still: 64 bit platforms are the default, so using a 32 bit type to pass allocation sizes is a bug of it's own.
  • when any additional patch accidentially mixes calls to this functions with normal realloc()/free() this can lead to random crashes
  • why bother at all with having a custom allocator?

Fixes #112

@schmonz
Copy link
Member

schmonz commented May 1, 2020

I find the motivation for doing this very strong. The sooner we can turn on Valgrind and ASan in CI, the sooner we can be systematically ratcheting safety upward.

@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch from c94f73d to 33a101b Compare May 8, 2020 14:27
@DerDakon
Copy link
Member Author

We have 2 possibilities: either merge #113 first, which renames the usages of alloc in cdb code, or we just remove it. Either way, we have to touch that anyway as otherwise the alloc there will be changed from the define to malloc, which would also affect the function parameter. IMHO the best thing is to just remove that entirely and use malloc() directly as there is no need to pass that as function pointer here. Afterwards the alloc(x) version of the define can be used again.

@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch from 316fa25 to c17daa8 Compare May 11, 2020 20:00
cdbmss.c Outdated
uint32 u;

if (!cdbmake_split(&c->cdbm,alloc)) return -1;
if (!cdbmake_split(&c->cdbm,malloc)) return -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous

@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch 2 times, most recently from c24c897 to cf0b2ac Compare May 12, 2020 18:49
@DerDakon DerDakon added this to the 1.09 milestone May 12, 2020
@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch from cf0b2ac to 0c56b6d Compare May 12, 2020 20:04
@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch from 865ed34 to 6727a89 Compare May 20, 2020 18:48
DerDakon added 3 commits May 23, 2020 00:08
Both of these functions are only called from a single place, where both pass in
the alloc() function. The parameter was also named alloc, which can cause
compiler warnings when -Wshadow is enabled. Simply remove the parameter, which
makes everything work exactly as before.
 * it's as simple as possible, probably intentional. This makes things like
   alloc_re() (i.e. realloc()) inefficient as it does not look if the thing it
   reallocates is already the last thing in the batch, and simply extends that,
   or if alloc_free() has the last block and adds things back to the pool
   (ideally with zeroing for security reasons).
 * it makes it basically impossible to detect overruns on early small
   allocations with tools like valgrind, address sanitizer or friends as
   those allocations never hit malloc(), so are never traced by any custom
   tools. Any overrun will just go into the next variable, untracable.
 * allocators do their own buffering, alignment, and so on. If they are broken,
   your system is severely screwed. And if we need more money^H^H^H^Hemory we
   will hit that buggy allocator anyway, making things harder to detect as it
   will only happen on "heavy" memory usage. This is not TruOS, SunOS 1.x or
   something like that anymore. We expect a reasonably sane OS. Everything else
   will get us screwed in much more severe ways.
 * it has shown issues when coming close to 4GiB allocations (#37, #109, older
   reports). DJB correctly says that an allocation of that size in your mailer
   shows that you have already done something wrong, but still: 64 bit
   platforms are the default, so using a 32 bit type to pass allocation sizes
   is a bug of it's own.
 * when any additional patch accidentially mixes calls to this functions with
   normal realloc()/free() this can lead to random crashes
 * why bother at all with having a custom allocator?
@DerDakon DerDakon force-pushed the Dakon-remove-alloc branch from 6727a89 to 1720e0b Compare May 22, 2020 22:25
@DerDakon DerDakon requested a review from leahneukirchen May 22, 2020 22:37
@DerDakon DerDakon merged commit 1720e0b into master May 23, 2020
@DerDakon DerDakon deleted the Dakon-remove-alloc branch May 23, 2020 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove the custom allocator (alloc.c)

5 participants