Mailing List Archive: ClamAV: devel

clamd hung up

Index | Next | Previous | View Threaded

jmaimon at ttec

Apr 27, 2004, 6:04 PM

Post #1 of 10 (1278 views)
Permalink

clamd hung up

Hello All,

In clamd/server-th.c why do we not wait to call pthread_mutex_unlock
until after reload_db?
ALSO: Isnt this a call to pthread_mutex_unlock after its already unlocked?

pthread_mutex_lock(&reload_mutex);
if(reload) {
reload = 0;
pthread_mutex_unlock(&reload_mutex);
/* Destroy the thread manager.
* This waits for all current tasks to end
*/
thrmgr_destroy(thr_pool);
root = reload_db(root, copt, FALSE);
if((thr_pool=thrmgr_new(max_threads, 30, scanner_thread)) ==
NULL) {
logg("!thrmgr_new failed\n");
pthread_mutex_unlock(&reload_mutex);
exit(-1);
}
#ifdef CLAMUKO
if(cfgopt(copt, "ClamukoScanOnLine") || cfgopt(copt,
"ClamukoScanOnAccess")) {
logg("Stopping and restarting Clamuko.\n");
pthread_kill(clamuko_pid, SIGUSR1);
pthread_join(clamuko_pid, NULL);
tharg->root = root;
pthread_create(&clamuko_pid, &clamuko_attr, clamukoth,
tharg);
}
#endif
} else {
pthread_mutex_unlock(&reload_mutex);
}
}

trog at uncon

Apr 28, 2004, 12:40 AM

Post #2 of 10 (1236 views)
Permalink

Re: clamd hung up [In reply to]

On Tue, 2004-04-27 at 16:34, Joe Maimon wrote:
> Hello All,
>
> In clamd/server-th.c why do we not wait to call pthread_mutex_unlock
> until after reload_db?

Because the 'reload_mutex' mutex protects the variable 'reload'. Thats
all.

> ALSO: Isnt this a call to pthread_mutex_unlock after its already unlocked?
>

No.

-trog

Attachments:

signature.asc (0.18 KB)

jmaimon at ttec

Apr 28, 2004, 3:02 AM

Post #3 of 10 (1246 views)
Permalink

Re: clamd hung up [In reply to]

Trog wrote:

>On Tue, 2004-04-27 at 16:34, Joe Maimon wrote:
>
>
>>Hello All,
>>
>>In clamd/server-th.c why do we not wait to call pthread_mutex_unlock
>>until after reload_db?
>>
>>
>
>Because the 'reload_mutex' mutex protects the variable 'reload'. Thats
>all.
>
>
>
You mean it doesnt protect against reload_db being called twice with a
possible race.

>>ALSO: Isnt this a call to pthread_mutex_unlock after its already unlocked?
>>
>>
>>
>
>No.
>
>
I dont understand.
Calls to pthread_mutex_[un]lock highlighted

*****pthread_mutex_lock(&reload_mutex);*****
if(reload) {
reload = 0;
******pthread_mutex_unlock(&reload_mutex); ****** (would it be better to
move this below reload_db?)
/* Destroy the thread manager.
* This waits for all current tasks to end
*/
thrmgr_destroy(thr_pool);
root = reload_db(root, copt, FALSE);
if((thr_pool=thrmgr_new(max_threads, 30, scanner_thread)) ==
NULL) {
logg("!thrmgr_new failed\n");
******pthread_mutex_unlock(&reload_mutex);*******
This unlock? Which lock does it correspond too?

>-trog
>
>
>

jmaimon at ttec

Apr 28, 2004, 3:04 AM

Post #4 of 10 (1235 views)
Permalink

Re: clamd hung up [In reply to]

BTW

I understand you are probably very busy and all this is more a work of
love than duty for you.

But still.

A) I am perfectly capable of reading man pthread_mutex_lock

In fact I did multiple time.

B) Obviously you already thought "NO" otherwise the code would reflect that,

I was hoping you would explain with a drop more detail what I seem to be
missing here.

Joe

Trog wrote:

>On Tue, 2004-04-27 at 16:34, Joe Maimon wrote:
>
>
>>Hello All,
>>
>>In clamd/server-th.c why do we not wait to call pthread_mutex_unlock
>>until after reload_db?
>>
>>
>
>Because the 'reload_mutex' mutex protects the variable 'reload'. Thats
>all.
>
>
>
>>ALSO: Isnt this a call to pthread_mutex_unlock after its already unlocked?
>>
>>
>>
>
>No.
>
>-trog
>
>
>

trog at uncon

Apr 28, 2004, 3:20 AM

Post #5 of 10 (1246 views)
Permalink

Re: clamd hung up [In reply to]

On Wed, 2004-04-28 at 11:01, Joe Maimon wrote:
> >Because the 'reload_mutex' mutex protects the variable 'reload'. Thats
> >all.
> >
> >
> >
> You mean it doesnt protect against reload_db being called twice with a
> possible race.

No, it doesn't, because:

1. Thats not it's job.
2. There isn't a 'possible race' (please provide a scenario if you
believe otherwise).

and, moving the unlock down creates a deadlock scenario.

> if((thr_pool=thrmgr_new(max_threads, 30, scanner_thread)) ==
> NULL) {
> logg("!thrmgr_new failed\n");
> ******pthread_mutex_unlock(&reload_mutex);*******
> This unlock? Which lock does it correspond too?
>

Ahh... this unlock does look like a cut-n-paste error. Having said that,
this is a fatal error path, and the next line is exit().

I'll fix that one.

Thanks
-trog

Attachments:

signature.asc (0.18 KB)

jmaimon at ttec

Apr 28, 2004, 4:15 AM

Post #6 of 10 (1248 views)
Permalink

Re: clamd hung up [In reply to]

Trog wrote:

>On Wed, 2004-04-28 at 11:01, Joe Maimon wrote:
>
>
>>>Because the 'reload_mutex' mutex protects the variable 'reload'. Thats
>>>all.
>>>
>>>
>>>
>>>
>
>
>
Ok. Thank you.
I was looking at this because clamd hung up on me right after logging
"No stats for Database check - forcing reload"
Whereby socket connections would open, but clamd wouldnt respond to
anything. not even a ping.

Unfortunately it happened at a bad time and I was unable to trace/gdb it.

It would make sense to suppose that steps were taken to ensure there was
only one thread executing reload_db with do_check == FALSE ?

or is struct cl_node *root thread local?

trog at uncon

Apr 28, 2004, 4:28 AM

Post #7 of 10 (1256 views)
Permalink

Re: clamd hung up [In reply to]

On Wed, 2004-04-28 at 12:14, Joe Maimon wrote:

> It would make sense to suppose that steps were taken to ensure there was
> only one thread executing reload_db with do_check == FALSE ?
>

It would make sense, which is why it is currently implemented that way.

-trog

Attachments:

signature.asc (0.18 KB)

jmaimon at ttec

Apr 28, 2004, 4:47 AM

Post #8 of 10 (1254 views)
Permalink

Re: clamd hung up [In reply to]

Trog wrote:

>On Wed, 2004-04-28 at 12:14, Joe Maimon wrote:
>
>
>
>>It would make sense to suppose that steps were taken to ensure there was
>>only one thread executing reload_db with do_check == FALSE ?
>>
>>
>>
>
>It would make sense, which is why it is currently implemented that way.
>
>-trog
>
>
>
I suppose by having thrmgr_destroy(thr_pool); wait for all worker
threads to finish?

If a thread refused to finish at that time would thrmgr_destroy not
return and clamd stop doing any more work even after accepting socket
connections?

2 more Questions.

from clamd/thrmgr.c in thrmgr_destroy()

/* wait for threads to exit */
if (threadpool->thr_alive > 0) {
if (pthread_cond_broadcast(&(threadpool->pool_cond)) != 0) {
pthread_mutex_unlock(&threadpool->pool_mutex);
return;
}
}
while (threadpool->thr_alive > 0) {
if (pthread_cond_wait (&threadpool->pool_cond,
&threadpool->pool_mutex) != 0) {
pthread_mutex_unlock(&threadpool->pool_mutex);
return;
}
}

1) man pthread_cond_broadcast states

RETURN VALUE
All condition variable functions return 0 on success and a
non-zero error code on error.

ERRORS
pthread_cond_init, pthread_cond_signal, pthread_cond_broadcast,
and pthread_cond_wait never return an
error code.

Does that mean that " if
(pthread_cond_broadcast(&(threadpool->pool_cond)) != 0) { " will never
be true?

2) Why do an "if" and then also a "while" on the same variable?

Thanks for your patience,

Joe

trog at uncon

Apr 28, 2004, 5:53 AM

Post #9 of 10 (1237 views)
Permalink

Re: clamd hung up [In reply to]

On Wed, 2004-04-28 at 12:46, Joe Maimon wrote:

> >
> I suppose by having thrmgr_destroy(thr_pool); wait for all worker
> threads to finish?

Yes.

>
> If a thread refused to finish at that time would thrmgr_destroy not
> return and clamd stop doing any more work even after accepting socket
> connections?

Yes, it would. And that would be a bug in the scanning engine.

>
> 2 more Questions.

>
> ERRORS
> pthread_cond_init, pthread_cond_signal, pthread_cond_broadcast,
> and pthread_cond_wait never return an
> error code.
>
> Does that mean that " if
> (pthread_cond_broadcast(&(threadpool->pool_cond)) != 0) { " will never
> be true?

No, because you only read the man page for a particular implementation.
Other implementations can return an error.

>
> 2) Why do an "if" and then also a "while" on the same variable?
>

Because thats a fundamental rule of using condition variables. To
protect against spurious wakeups.

-trog

Attachments:

signature.asc (0.18 KB)

jmaimon at ttec

Apr 28, 2004, 6:20 AM

Post #10 of 10 (1252 views)
Permalink

Re: clamd hung up [In reply to]

Trog wrote:

>On Wed, 2004-04-28 at 12:46, Joe Maimon wrote:
>
>
>
>>I suppose by having thrmgr_destroy(thr_pool); wait for all worker
>>threads to finish?
>>
>>
>
>Yes.
>
>
>
>>If a thread refused to finish at that time would thrmgr_destroy not
>>return and clamd stop doing any more work even after accepting socket
>>connections?
>>
>>
>
>Yes, it would. And that would be a bug in the scanning engine.
>
>
>
>>2 more Questions.
>>
>>
>
>
>
>>ERRORS
>> pthread_cond_init, pthread_cond_signal, pthread_cond_broadcast,
>>and pthread_cond_wait never return an
>> error code.
>>
>>Does that mean that " if
>>(pthread_cond_broadcast(&(threadpool->pool_cond)) != 0) { " will never
>>be true?
>>
>>
>
>No, because you only read the man page for a particular implementation.
>Other implementations can return an error.
>
>
>
>>2) Why do an "if" and then also a "while" on the same variable?
>>
>>
>>
>
>Because thats a fundamental rule of using condition variables. To
>protect against spurious wakeups.
>
>-trog
>
>
>
Thanks for the thought food,

Your,
Joe.

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads