Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

sc-allocator: Do not panic on invalid header pointer #13925

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Apr 14, 2023

We should not panic on an invalid header pointer and instead return an error. It is possible that the application modifies the header pointer illegally, but then we should return an error instead of panicking.

Closes: #13924

We should not panic on an invalid header pointer and instead return an error. It is possible that
the application modifies the header pointer illegally, but then we should return an error instead of
panicking.
@bkchr bkchr added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. labels Apr 14, 2023
@bkchr bkchr requested a review from a team April 14, 2023 20:48
@bkchr bkchr requested a review from koute as a code owner April 14, 2023 20:48
@michalkucharczyk michalkucharczyk requested a review from a team April 14, 2023 22:05
if (u64::from(header_ptr) + u64::from(order.size()) + u64::from(HEADER_SIZE)) >
mem.size()
{
return Err(error("Invalid header pointer detected"))
Copy link
Member

@ggwpez ggwpez Apr 15, 2023

Choose a reason for hiding this comment

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

This will poison the heap now. Could you please add this to the test?

I assume this is intended as per doc: Once the allocator has returned an error all subsequent requests will return an error..

Copy link
Member Author

Choose a reason for hiding this comment

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

We will abort on the first allocation error any way.

@bkchr bkchr merged commit f1e2fa4 into master Apr 15, 2023
@bkchr bkchr deleted the bkchr-freeing-bump-do-not-panic branch April 15, 2023 21:27
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
We should not panic on an invalid header pointer and instead return an error. It is possible that
the application modifies the header pointer illegally, but then we should return an error instead of
panicking.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PVF could assert! the client by crafting a FreeingBumpHeapAllocator freelist header
4 participants