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

Safe allocation wrappers #754

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Safe allocation wrappers #754

merged 4 commits into from
Jan 28, 2020

Conversation

michael-grunder
Copy link
Collaborator

Adds allocation wrappers that invokes a defined OOM handler.

My goal was the smallest surface area of changes possible. Once we merge this it might be prudent to replace every malloc/free call with our wrappers.

@michael-grunder michael-grunder changed the title Safe alloc Safe allocation wrappers Jan 23, 2020
@lamby
Copy link
Contributor

lamby commented Jan 23, 2020

Just quoting the last parts of the conversation in a merged PR from @michael-grunder

@lamby I totally understand not wanting such a huge change just for a patch.

Thanks... and I think the changes in this PR would work for me. 👍

@lamby
Copy link
Contributor

lamby commented Jan 27, 2020

Would be great to get some more eyes on this. :)

Copy link
Contributor

@mnunberg mnunberg left a comment

Choose a reason for hiding this comment

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

are we using asprintf and friends anywhere?

@michael-grunder
Copy link
Collaborator Author

michael-grunder commented Jan 28, 2020

are we using asprintf and friends anywhere?

No, but sds has something similar (e.g. sdscatfmt) although it appears that we're testing for NULL after every allocation/reallocation.

@michael-grunder michael-grunder merged commit 669ac9d into master Jan 28, 2020
@michael-grunder
Copy link
Collaborator Author

Merged, thanks everyone!

alloc.c Show resolved Hide resolved
@ch1aki ch1aki mentioned this pull request Jan 29, 2020
michael-grunder added a commit that referenced this pull request Mar 13, 2020
Create allocation wrappers with a configurable OOM handler (defaults to abort()).

See #752, #747
valentinogeron pushed a commit to valentinogeron/hiredis that referenced this pull request Mar 17, 2020
Create allocation wrappers with a configurable OOM handler (defaults to abort()).

See redis#752, redis#747

Conflicts:
	Makefile
	adapters/libevent.h
@@ -57,7 +58,7 @@ static unsigned int callbackHash(const void *key) {

static void *callbackValDup(void *privdata, const void *src) {
((void) privdata);
redisCallback *dup = malloc(sizeof(*dup));
redisCallback *dup = hi_malloc(sizeof(*dup));

Choose a reason for hiding this comment

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

Can still result in a SEGFAULT,
If hi_malloc returns NULL -> next call to memcpy will SEGFAULT

Copy link
Contributor

Choose a reason for hiding this comment

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

Just waking up here but how can hi_malloc return NULL if HIREDIS_OOM_HANDLER is called? (eg. _exit?)

Choose a reason for hiding this comment

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

@lamby yeah, I forgot to redo the comment, talked about it already with @michael-grunder
either way personally I'm against doing abort() if we can handle it, for example when doing callback-allocation, we can return error instead of abort, remember that this is a 3rd-party library for most users, and crashing someone's application due to internal errors is not the way to do things IMHO

@michael-grunder michael-grunder deleted the safe-alloc branch July 11, 2020 18:07
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.

5 participants