#3548 crash in tclAsync.c:Tcl_AsyncDelete()

[Anonymous]

hmmm 8.4.13 is obsolete but I only downloaded it a few
days ago :-)

OK, to business. I am running Tcl 8.4.13 inside
AOLServer 4.5.0.

patch is...

james@redhat::~/tcl/tcl8.4.13/generic [295] diff -C3
tclAsync.c.orig tclAsync.c
*** tclAsync.c.orig 2006-10-23 16:31:11.000000000 +1000
--- tclAsync.c 2006-10-23 16:32:27.000000000 +1000
***************
*** 292,305 ****
if (tsdPtr->firstHandler == NULL) {
tsdPtr->lastHandler = NULL;
}
} else {
prevPtr = tsdPtr->firstHandler;
while (prevPtr->nextPtr != asyncPtr) {
prevPtr = prevPtr->nextPtr;
}
! prevPtr->nextPtr = asyncPtr->nextPtr;
! if (tsdPtr->lastHandler == asyncPtr) {
! tsdPtr->lastHandler = prevPtr;
}
}
Tcl_MutexUnlock(&tsdPtr->asyncMutex);
--- 292,311 ----
if (tsdPtr->firstHandler == NULL) {
tsdPtr->lastHandler = NULL;
}
+ } else if (tsdPtr->firstHandler == NULL) {
+ /* empty list */
} else {
prevPtr = tsdPtr->firstHandler;
while (prevPtr->nextPtr != asyncPtr) {
prevPtr = prevPtr->nextPtr;
}
! if (prevPtr == NULL)
! ; /* wasn't in the list */
! else {
! prevPtr->nextPtr = asyncPtr->nextPtr;
! if (tsdPtr->lastHandler == asyncPtr) {
! tsdPtr->lastHandler = prevPtr;
! }
}
}
Tcl_MutexUnlock(&tsdPtr->asyncMutex);

That patch seems to work, so I assume that in my case
the 'Tcl_AsyncHandler async' is not in the list. How
that happens is a question for another day.

I have the following setup:

config.status
# This directory was configured as follows,
# on host redhat.home.jmsh.net:
#
# ./configure --prefix=/usr/local/tcl --enable-threads

Linux redhat.home.jmsh.net 2.4.20-8 #1 Thu Mar 13
17:54:28 EST 2003 i686 i686 i386 GNU/Linux

actually this is REDHAT 9 - nice and old

A quick Google search will reveal a German lady
Michelle having a similar problem a year or so ago.

Keep in mind that I am not really sure what is
happening in the code here, I was just trying to stop
it crashing. In this case Tcl_AsyncDelete() was being
called an instant before pthread_exit(). That would
suggest to me that I have introduced a memory leak. I
assume that you would know the consequences and
therefore the proper response to this function
'failing'. Maybe an exception rather than the silent
failure I have introduced.

Let me know if I can be of assistance.

Thanks,
James Burton
james@delete.jmsh.net

[Joe Mistachkin]

Logged In: YES
user_id=113501

The code in Tcl_AsyncDelete suffers from a general lack of
robustness in the branch of code that searches for the
AsyncHandler. This bug appears to be present in both 8.4
and 8.5.

[Nobody/Anonymous]

Logged In: NO

Oops, I've just found another logic bug. Make sure the while
loop has
a NULL test so it doesn't run off the end of the loop.

My patch now looks like the following:

Thanks,
James

*** tclAsync.c.orig 2006-10-23 16:31:11.000000000 +1000
--- tclAsync.c 2006-11-04 01:07:33.000000000 +1100
***************
*** 292,305 ****
if (tsdPtr->firstHandler == NULL) {
tsdPtr->lastHandler = NULL;
}
} else {
prevPtr = tsdPtr->firstHandler;
! while (prevPtr->nextPtr != asyncPtr) {
prevPtr = prevPtr->nextPtr;
}
! prevPtr->nextPtr = asyncPtr->nextPtr;
! if (tsdPtr->lastHandler == asyncPtr) {
! tsdPtr->lastHandler = prevPtr;
}
}
Tcl_MutexUnlock(&tsdPtr->asyncMutex);
--- 292,311 ----
if (tsdPtr->firstHandler == NULL) {
tsdPtr->lastHandler = NULL;
}
+ } else if (tsdPtr->firstHandler == NULL) {
+ /* empty list */
} else {
prevPtr = tsdPtr->firstHandler;
! while (prevPtr != NULL && prevPtr->nextPtr != asyncPtr) {
prevPtr = prevPtr->nextPtr;
}
! if (prevPtr == NULL)
! ; /* wasn't in the list */
! else {
! prevPtr->nextPtr = asyncPtr->nextPtr;
! if (tsdPtr->lastHandler == asyncPtr) {
! tsdPtr->lastHandler = prevPtr;
! }
}
}
Tcl_MutexUnlock(&tsdPtr->asyncMutex);

[Zoran Vasiljevic]

Logged In: YES
user_id=95086

I'm aware that this (Tcl_Async) code is broken.
I will look into your patch in more detail.
Thank you.

[Joe Mistachkin]

Logged In: YES
user_id=113501
Originator: NO

Still not fixed in HEAD. I have an alternative patch for this:

Index: generic/tclAsync.c

RCS file: /cvsroot/tcl/tcl/generic/tclAsync.c,v
retrieving revision 1.10
diff -b -u -r1.10 tclAsync.c
--- generic/tclAsync.c 11 Jul 2006 14:29:14 -0000 1.10
+++ generic/tclAsync.c 24 Jul 2007 09:51:42 -0000
@@ -286,13 +286,18 @@
}
} else {
prevPtr = tsdPtr->firstHandler;
- while (prevPtr->nextPtr != asyncPtr) {
+ while (prevPtr->nextPtr != NULL
+ && prevPtr->nextPtr != asyncPtr) {
prevPtr = prevPtr->nextPtr;
}
+ if (prevPtr->nextPtr == asyncPtr) {
prevPtr->nextPtr = asyncPtr->nextPtr;
if (tsdPtr->lastHandler == asyncPtr) {
tsdPtr->lastHandler = prevPtr;
}
+ } else {
+ panic("Tcl_AsyncDelete: cannot find async handler");
+ }
}
}
Tcl_MutexUnlock(&tsdPtr->asyncMutex);

[Don Porter]

Logged In: YES
user_id=80530
Originator: NO

So Tcl_AsyncDelete() performs two tasks.
It removes an AsyncHandler from the
per-thread list of handlers, and it
frees the AsyncHandler.

The bug report appears to report trouble
when Tcl_AsyncDelete(token) gets called
in the wrong thread -- a thread different
from the one that made the Tcl_AsyncCreate()
call that returned token in the first place.

Callers shouldn't do that. But in case they
do, it does look like a Panic is a better
response than than either a hang or a crash
with no message.

I'd feel much better if vasiljevic were
making this analysis instead of me.

[Zoran Vasiljevic]

Logged In: YES
user_id=95086
Originator: NO

I will look into this tomorrow.

Cheers
Zoran

[Zoran Vasiljevic]

Logged In: YES
user_id=95086
Originator: NO

So... I finally got time to look at this one.

Failure to locate the async token in (thread's own) list of generated tokens is
a sign of a big problem and the only (right) thing to do at that place is to
Tcl_Panic.

So, you just cannot create the token in one thread and delete it in another.
The thread that created the token must be the same thread that destroys it.

You can (and will) need to pass this token to some other thread and issue
Tcl_Async_Mark on that token that would mark the event in the thread that
created the token. BUT be aware that this thread is still arround at that time!
As, if it is already gone, you will crash. Tcl does not maintain (should it?) a
list of threads generated by its own Tcl_CreateThread() command, otherwise
we could prove originating thread existence and Tcl_Panic more clearly...
In any case, Tcl_Panic is inevitable, just error logging would be better.

Patch seems allright for me.
I will put this into both HEAD and core-8-4-branch.

Zoran

[Joe Mistachkin]

Logged In: YES
user_id=113501
Originator: NO

What about core-8-5-branch (since HEAD is now 8.6)?

[Zoran Vasiljevic]

Logged In: YES
user_id=95086
Originator: NO

Oooops!