[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Threadpool: Initialize new dynamic workers



On 07.09.2011 14:22, Jiri Denemark wrote:
> On Wed, Sep 07, 2011 at 14:11:14 +0200, Michal Privoznik wrote:
>> Although we were initializing worker threads during pool creating,
>> we missed this during virThreadPoolSendJob. This bug led to segmenation
>> fault as worker thread free() given argument.
>> ---
>>  src/util/threadpool.c |   13 ++++++++++++-
>>  1 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/util/threadpool.c b/src/util/threadpool.c
>> index 70a75c0..6210b00 100644
>> --- a/src/util/threadpool.c
>> +++ b/src/util/threadpool.c
>> @@ -286,6 +286,7 @@ int virThreadPoolSendJob(virThreadPoolPtr pool,
>>                           void *jobData)
>>  {
>>      virThreadPoolJobPtr job;
>> +    struct virThreadPoolWorkerData *data = NULL;
>>  
>>      virMutexLock(&pool->mutex);
>>      if (pool->quit)
>> @@ -298,10 +299,19 @@ int virThreadPoolSendJob(virThreadPoolPtr pool,
>>              goto error;
>>          }
>>  
>> +        if (VIR_ALLOC(data) < 0) {
>> +            pool->nWorkers--;
>> +            virReportOOMError();
>> +            goto error;
>> +        }
>> +
>> +        data->pool = pool;
>> +        data->cond = &pool->cond;
>> +
>>          if (virThreadCreate(&pool->workers[pool->nWorkers - 1],
>>                              true,
>>                              virThreadPoolWorker,
>> -                            pool) < 0) {
>> +                            data) < 0) {
>>              pool->nWorkers--;
> 
> You need to VIR_FREE(data) here...
> 
>>              goto error;
>>          }
>> @@ -336,6 +346,7 @@ int virThreadPoolSendJob(virThreadPoolPtr pool,
>>      return 0;
>>  
>>  error:
>> +    VIR_FREE(data);
> 
> ...instead of here because you can get to error label after the worker thread
> was successfully created (in case allocating job fails) so either data would
> be freed twice or the worker would read from memory that was already freed.
> 
>>      virMutexUnlock(&pool->mutex);
>>      return -1;
>>  }
> 
> ACK with this fixed.
> 
> Jirka


Thanks, pushed with that fixed.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]