Skip to content

Fix callReplyParseCollection memleak when use AutoMemory#9446

Merged
oranagra merged 2 commits intoredis:unstablefrom
chenyang8094:fix-module-reply-memleak
Sep 9, 2021
Merged

Fix callReplyParseCollection memleak when use AutoMemory#9446
oranagra merged 2 commits intoredis:unstablefrom
chenyang8094:fix-module-reply-memleak

Conversation

@chenyang8094
Copy link
Contributor

@chenyang8094 chenyang8094 commented Sep 2, 2021

When parsing an array type reply, ctx will be lost when recursively parsing its elements, which will cause a memory leak in automemory mode.

static void callReplyParseCollection(ReplyParser *parser, CallReply *rep, size_t len, const char *proto, size_t elements_per_entry) {
    rep->len = len;
    rep->val.array = zcalloc(elements_per_entry * len * sizeof(CallReply));
    for (size_t i = 0; i < len * elements_per_entry; i += elements_per_entry) {
        for (size_t j = 0 ; j < elements_per_entry ; ++j) {
            // here lost the ctx
            parseReply(parser, rep->val.array + i + j); 
            rep->val.array[i + j].flags |= REPLY_FLAG_PARSED;
            rep->val.array[i + j].private_data = rep->private_data;
            if (rep->val.array[i + j].flags & REPLY_FLAG_RESP3) {
                /* If one of the sub-replies is RESP3, then the current reply is also RESP3. */
                rep->flags |= REPLY_FLAG_RESP3;
            }
        }
    }
    rep->proto = proto;
    rep->proto_len = parser->curr_location - proto;
}

This is a result of the changes in #9202

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Sep 2, 2021
@MeirShpilraien
Copy link

@chenyang8094 nice catch, I guess you mean that the RM_CreateStringFromCallReply is leaking?
I believe we should also add a test that verifies this fix, @oranagra what do you think?

@oranagra
Copy link
Member

oranagra commented Sep 6, 2021

Yes, let's make sure to cover as much as we can in these code paths with tests for auto-memory issues.
@chenyang8094 do you wanna handle that in this PR?

@chenyang8094
Copy link
Contributor Author

@chenyang8094 nice catch, I guess you mean that the RM_CreateStringFromCallReply is leaking?
I believe we should also add a test that verifies this fix, @oranagra what do you think?

@MeirShpilraien Yes, there is leak in RM_CreateStringFromCallReply.

@chenyang8094
Copy link
Contributor Author

Yes, let's make sure to cover as much as we can in these code paths with tests for auto-memory issues.
@chenyang8094 do you wanna handle that in this PR?

Test has been added. @MeirShpilraien @oranagra

@MeirShpilraien
Copy link

Thanks @chenyang8094 , looks good.

@oranagra oranagra merged commit bc0c22f into redis:unstable Sep 9, 2021
@sundb
Copy link
Collaborator

sundb commented Sep 9, 2021

I have a test failure in daily ci. Looking at the git logs it could be caused by this pr.
https://github.com/sundb/redis/runs/3555257411?check_suite_focus=true

@MeirShpilraien
Copy link

@sundb this should fix it: #9481

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants