Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slab pool boundary guard #585

Merged

Conversation

nolan-veed
Copy link
Contributor

Why

For #582

What

  • Used sizeof(AlignMax) as boundary size.
  • Updated PoisonOps.
  • Updated PoolImpl.

Testing

  • Wrote tests can be un-ignored to cause a panic.

@gavv
Copy link
Member

gavv commented Oct 1, 2023

Thanks for PR, will review in upcoming days! Android timeout is a known issue, I've restarted CI.

@gavv gavv added the ready for review Pull request can be reviewed label Oct 1, 2023
@nolan-veed nolan-veed marked this pull request as ready for review October 1, 2023 16:33
@nolan-veed
Copy link
Contributor Author

Currently, I put a boundary before and after user the data area. But, it just occurred to me that if the user data area is already padded (max-alignment) - should we also check that for buffer overruns.

Copy link
Member

@gavv gavv left a comment

Choose a reason for hiding this comment

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

Thanks again for the patch, looks fine. Here are some comments.

src/internal_modules/roc_core/poison_ops.h Outdated Show resolved Hide resolved
src/internal_modules/roc_core/poison_ops.h Outdated Show resolved Hide resolved
src/internal_modules/roc_core/poison_ops.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_core/pool_impl.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_core/pool_impl.cpp Outdated Show resolved Hide resolved
src/tests/roc_core/test_pool.cpp Outdated Show resolved Hide resolved
src/tests/roc_core/test_pool.cpp Outdated Show resolved Hide resolved
src/tests/roc_core/test_pool.cpp Outdated Show resolved Hide resolved
@gavv gavv added needs revision Pull request should be revised by its author contribution A pull-request by someone else except maintainers and removed ready for review Pull request can be reviewed labels Oct 2, 2023
@nolan-veed nolan-veed requested a review from gavv October 2, 2023 23:00
@gavv gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Oct 3, 2023
src/internal_modules/roc_core/pool_impl.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_core/pool_impl.cpp Outdated Show resolved Hide resolved
src/internal_modules/roc_core/pool_impl.h Outdated Show resolved Hide resolved
src/internal_modules/roc_core/pool.h Outdated Show resolved Hide resolved
@gavv gavv added needs revision Pull request should be revised by its author and removed ready for review Pull request can be reviewed labels Oct 3, 2023
@nolan-veed nolan-veed requested a review from gavv October 3, 2023 16:31
@gavv gavv added ready for review Pull request can be reviewed and removed needs revision Pull request should be revised by its author labels Oct 3, 2023
@gavv gavv merged commit 4c62abe into roc-streaming:develop Oct 4, 2023
31 checks passed
@gavv gavv removed the ready for review Pull request can be reviewed label Oct 4, 2023
@gavv
Copy link
Member

gavv commented Oct 4, 2023

Thanks for update!

Here is a small follow-up commit: c25b331

It adds name to other panics, and fixes small bug:

if ((flags_ & PoolFlag_PanicOnOverflow) == 1) {

will work only if PoolFlag_PanicOnOverflow is equal to 1, so it's replaced with:

if ((flags_ & PoolFlag_PanicOnOverflow) != 0) {

@gavv gavv added this to the 0.3.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution A pull-request by someone else except maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants