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

[BUG] A use after free bug in replicationCron #8797

Closed
Yunlongs opened this issue Apr 16, 2021 · 3 comments · Fixed by #8799
Closed

[BUG] A use after free bug in replicationCron #8797

Yunlongs opened this issue Apr 16, 2021 · 3 comments · Fixed by #8799

Comments

@Yunlongs
Copy link

Describe the bug
File: src/replication.c

In function replicationCron(), slave is freed by freeClient(slave) at line 3,404.
But the freed slave is dereferenced at line 3,410 by slave->replstate.

        while((ln = listNext(&li))) {
            client *slave = ln->value;

            if (slave->replstate == SLAVE_STATE_ONLINE) {
                if (slave->flags & CLIENT_PRE_PSYNC)
                    continue;
                if ((server.unixtime - slave->repl_ack_time) > server.repl_timeout) {
                    serverLog(LL_WARNING, "Disconnecting timedout replica (streaming sync): %s",
                          replicationGetSlaveName(slave));
                    freeClient(slave);  /* ------> Freed Here */
                }
            }

            if (slave->replstate == SLAVE_STATE_WAIT_BGSAVE_END && server.rdb_child_type == RDB_CHILD_TYPE_SOCKET) {
             /* ----> Used Here!*/

It causes a use after free.

Expected behavior

After the slave is freed, the freed chunk could be allocated by other objects and rewrite the value of slave->replstate,
it could cause crash or others.

I think we should set a new variable to accept the value of slave->replstate before the first if branch, and use the new variable later.

@oranagra oranagra added this to To do in 6.2 via automation Apr 16, 2021
@oranagra
Copy link
Member

Thanks @Yunlongs
This is new code, Wanna make a PR to solve it?

@Yunlongs
Copy link
Author

Thanks @Yunlongs
This is new code, Wanna make a PR to solve it?

My network proxy is something wrong, i cannot subimt a PR now..
I need some time to fix it. Or can you just fix this bug?

@oranagra
Copy link
Member

Yeah, no worries. If you can't, we'll fix soon.

guybe7 added a commit to guybe7/redis that referenced this issue Apr 16, 2021
@oranagra oranagra removed this from To do in 6.2 Apr 18, 2021
oranagra pushed a commit to oranagra/redis that referenced this issue May 3, 2021
oranagra pushed a commit to oranagra/redis that referenced this issue Jul 21, 2021
oranagra pushed a commit that referenced this issue Jul 21, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this issue Sep 8, 2021
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 a pull request may close this issue.

2 participants