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

Fix reply map value memory leak #25

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

siavash119
Copy link
Contributor

redis/hiredis-py#175

not sure why PyDict_SetItem is returning -1 even though the assignment seems to be successful. But it was skipping over the Py_DECREF calls causing a memory leak.

@mkmkme
Copy link
Collaborator

mkmkme commented Nov 5, 2024

Hey,

Thanks for your contribution! Could you explain your message a bit though?

not sure why PyDict_SetItem is returning -1 even though the assignment seems to be successful

The assignment is not guaranteed to be successful. It will fail e.g. if self->pending object is of a type that can't be used as a dict key.

@siavash119
Copy link
Contributor Author

You're right, my mistake; PyDict_SetItem is not returning -1 here normally. The leak, though, is it's returning 0 because the PyDict_SetItem is successful, but the Py_DECREF is only called if unsuccessful, therefore keeping an extra ref to the obj in memory. As per the c api docs, "(PyDict_SetItem) does not steal a reference to val." So a Py_DECREF is required even on successful completion.

if (PyDict_SetItem(parent, self->pendingObject, obj) < 0) {
Py_DECREF(obj);
Py_DECREF(self->pendingObject);
self->pendingObject = NULL;
return NULL;
}

@mkmkme
Copy link
Collaborator

mkmkme commented Nov 7, 2024

Thank you for a great catch! I was mistakenly thinking PyDict_SetItem would take ownership in case the call succeeds but this is indeed not the case.

@mkmkme mkmkme merged commit 898c3c8 into valkey-io:main Nov 7, 2024
34 checks passed
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.

2 participants