Skip to content

Use the freed memory in readHandler (Redis-benchmark.c) #669

Open
NanXiao opened this Issue Sep 13, 2012 · 6 comments

2 participants

@NanXiao
NanXiao commented Sep 13, 2012

Hi, antirez:

  in readHandler (Redis-benchmark.c);

 while(c->pending) {
        if (redisGetReply(c->context,&reply) != REDIS_OK) {
            fprintf(stderr,"Error: %s\n",c->context->errstr);
            exit(1);
        }
        if (reply != NULL) {
            ......
            c->pending--;
            /* In clientDone function, the memory of c has been freed,
               but in  while(c->pending), access the memory again, and it will cause core dump!! */
            if (c->pending == 0) clientDone(c);
        } else {
            break;
        }

     I think the code should be:
    if (c->pending == 0) 
    {
            clientDone(c);
            break:;
    }

   Could you help to check this? Thanks very much!!

Best Regards
Nan Xiao

@moreaki
moreaki commented Oct 9, 2012

I fail to see how there would be a change with this in the code path. Could you elaborate a bit more on the claimed "use the free'd memory", please? Unless I misread your change, the while(c->pending) {...} loop terminates in the case of (c->pending == 0), which is the endroit where you added the additional break.

@NanXiao
NanXiao commented Oct 10, 2012

Hi, moreaki:

The code of current implementation:

if (c->pending == 0) clientDone(c);
In clientDone function, the c's memory has been freed, then the loop will continue: while(c->pending). The memory of c has been freed now, so c->pending is invalid (c is an invalid pointer now), and this will cause memory dump in some platforams(eg: Solaris).

So I think the code should be modified as:
if (c->pending == 0)
{
clientDone(c);
break;
}
and this will not lead to while(c->pending).

Could you help to check this? Thanks very much!!

Best Regards
Nan Xiao

@moreaki
moreaki commented Oct 10, 2012

You are correct with your analysis, Nan Xiao. A look at clientDone():

static void clientDone(client c) {
    if (config.requests_finished == config.requests) {
        freeClient(c);
        aeStop(config.el);
        return;
    }
    if (config.keepalive) {
        resetClient(c);
    } else {
        config.liveclients--;
        createMissingClients(c);
        config.liveclients++;
        freeClient(c);
    }
}

Your case happens in case of (config.requests_finished == config.requests) being true or (config.keepalive) being false. Normally, the process of code fixes works by modifying the code in the git repository and adding a pull request to te upstream maintainer. Do you care to provide a fix with a subsequent pull request?

Thank you and best regards.

@NanXiao
NanXiao commented Oct 10, 2012

Hi, moreaki:
Thanks very much for your reply. I have sent a pull requet:#707.

Best Regards
Nan Xiao

@moreaki
moreaki commented Oct 10, 2012

G'day Nan Xiao.

Excellent. As I have just seen, it has been merged already. Thanks for your catch and the patch.

Take care.

@NanXiao
NanXiao commented Oct 11, 2012

Hi, moreaki:

Welcome!

Best Regards
Nan Xiao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.