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

Module interface and use of static robj #8512

Open
JimB123 opened this issue Feb 18, 2021 · 1 comment
Open

Module interface and use of static robj #8512

JimB123 opened this issue Feb 18, 2021 · 1 comment

Comments

@JimB123
Copy link
Collaborator

JimB123 commented Feb 18, 2021

The module API uses robj for passing keys. By using robj (rather than an sds or char*) this provides the ability for the module to save the string via ref-counts. See: RM_RetainString.

In Redis 6.2 RC2, I'm seeing the use of stack allocated robj being passed to a module interface. Of course this won't work if the module attempts to retain the value.

Code reference:
https://github.com/redis/redis/blame/303465af35c13691f989b3400b028a94df1235d4/src/rdb.c#L2554-L2582

There are a couple of other uses of a stack-based robj in this file, but I can't immediately see that they are wrong. Any use of a stack-based robj is risky as it's hard to know how the robj is used by called routines. It's also hard to predict future modifications to the code.

I suggest that a real heap-based robj should be used in almost all cases. It should be readily apparent that a stack-based robj is only used locally. Note: this implies that the referenced string must also be heap-based and that the string will not be deleted independently from the robj.

@oranagra
Copy link
Member

We identified some of these problems before redis 6.0 went out. The main one was the key names in RM_GetKeyNameFromIO that suffered from an optimization in rdb.c, but considering this optimization resulted in huge speed improvement for the rdb parsing, and the fact that API wasn't highly used, we decided to create RM_HoldString that can create a new heap allocation, and depreciate RM_RetainString.

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

No branches or pull requests

2 participants