Skip to content

Fix undefined behavior in schedule.cc operator delete#1311

Open
boomanaiden154 wants to merge 1 commit intosteveicarus:masterfrom
boomanaiden154:fix-ub-slab-alloc
Open

Fix undefined behavior in schedule.cc operator delete#1311
boomanaiden154 wants to merge 1 commit intosteveicarus:masterfrom
boomanaiden154:fix-ub-slab-alloc

Conversation

@boomanaiden154
Copy link
Copy Markdown

Schedule.cc currently makes use of a custom slab allocator by having some structs specify a custom operator new/operator delete that call into the slab allocator. However, this setup currently relies on C++ UB, namely that writes that happen in operator delete are persisted afterwards. The slab allocator inside the free_slab function uses the memory of the object being freed to store allocator metadata, which is not allowed given the rules around operator delete.

This patch changes the internal storage of slab_allocator to a struct rather than a union so we can only return the actual storage when allocating an object and there is a header for each object that the allocator can use for metadata without writes to it being UB.

Schedule.cc currently makes use of a custom slab allocator by having
some structs specify a custom operator new/operator delete that call
into the slab allocator. However, this setup currently relies on C++ UB,
namely that writes that happen in operator delete are persisted
afterwards. The slab allocator inside the free_slab function uses the
memory of the object being freed to store allocator metadata, which is
not allowed given the rules around operator delete.

This patch changes the internal storage of slab_allocator to a struct
rather than a union so we can only return the actual storage when
allocating an object and there is a header for each object that the
allocator can use for metadata without writes to it being UB.
@boomanaiden154
Copy link
Copy Markdown
Author

This does increase total memory usage by a bit now that we have to allocate data for the header explicitly. However, this didn't seem to have a noticeable impact on performance in the cases I evaluated.

I also tried going into schedule.cc, marking operator delete protected/deleted, and adding a new virtual destroy method that would call into the slab allocator free_slab method. That would avoid the heap memory usage regression, but caused a runtime performance regression due to what I assume was different devirtualization/inlining behavior from the compiler that I was using, which would likely take building with a PGO profile to fix, which I would rather avoid.

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.

1 participant