From 192fce9c01c4ee3827aa8f54967764c18bdd5dca Mon Sep 17 00:00:00 2001 From: sebres Date: Tue, 22 Apr 2014 13:25:41 +0000 Subject: Memory leak after thread exit, fixed (alloc cache released by exit), belong to ticket [3493120] Moved over to branch bug-3493120. This is not ready for the core-8-5-branch. Segfaults all over the place in a thread-enabled build on a CentOS system. --- generic/tclInt.h | 1 + generic/tclThread.c | 8 ++++++-- generic/tclThreadAlloc.c | 27 +++++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/generic/tclInt.h b/generic/tclInt.h index 1348340..00b246b 100644 --- a/generic/tclInt.h +++ b/generic/tclInt.h @@ -2550,6 +2550,7 @@ MODULE_SCOPE void TclFinalizeObjects(void); MODULE_SCOPE void TclFinalizePreserve(void); MODULE_SCOPE void TclFinalizeSynchronization(void); MODULE_SCOPE void TclFinalizeThreadAlloc(void); +MODULE_SCOPE void TclFinalizeThreadAllocThread(void); MODULE_SCOPE void TclFinalizeThreadData(void); MODULE_SCOPE void TclFinalizeThreadObjects(void); MODULE_SCOPE double TclFloor(mp_int *a); diff --git a/generic/tclThread.c b/generic/tclThread.c index 8384107..d6b5bcb 100644 --- a/generic/tclThread.c +++ b/generic/tclThread.c @@ -338,8 +338,9 @@ Tcl_ConditionFinalize( * * TclFinalizeThreadData -- * - * This function cleans up the thread-local storage. This is called once - * for each thread. + * This function cleans up the thread-local storage. Secondary, it cleans + * thread alloc cache. + * This is called once for each thread before thread exits. * * Results: * None. @@ -354,6 +355,9 @@ void TclFinalizeThreadData(void) { TclpFinalizeThreadDataThread(); +#if defined(TCL_THREADS) && defined(USE_THREAD_ALLOC) + TclFinalizeThreadAllocThread(); +#endif } /* diff --git a/generic/tclThreadAlloc.c b/generic/tclThreadAlloc.c index 2e74fa7..106e908 100644 --- a/generic/tclThreadAlloc.c +++ b/generic/tclThreadAlloc.c @@ -1012,6 +1012,33 @@ TclFinalizeThreadAlloc(void) TclpFreeAllocCache(NULL); } +/* + *---------------------------------------------------------------------- + * + * TclFinalizeThreadAllocThread -- + * + * This procedure is used to destroy single thread private resources used + * in this file. + * Called in TclpFinalizeThreadData when a thread exits (Tcl_FinalizeThread). + * + * Results: + * None. + * + * Side effects: + * None. + * + *---------------------------------------------------------------------- + */ + +void +TclFinalizeThreadAllocThread(void) +{ + Cache *cachePtr = TclpGetAllocCache(); + if (cachePtr != NULL) { + TclpFreeAllocCache(cachePtr); + } +} + #else /* !(TCL_THREADS && USE_THREAD_ALLOC) */ /* *---------------------------------------------------------------------- -- cgit v0.12 From 190e2f09e8fa5bc975ba496a5544b4dd853378f0 Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 22 Apr 2014 14:57:44 +0000 Subject: Add refcounting and preservation to [testchannel transform] to stop segfault in test iogt-2.4. --- generic/tclIOGT.c | 122 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 47 deletions(-) diff --git a/generic/tclIOGT.c b/generic/tclIOGT.c index eed21fb..beddf4f 100644 --- a/generic/tclIOGT.c +++ b/generic/tclIOGT.c @@ -210,7 +210,27 @@ struct TransformChannelData { * a transformation of incoming data. Also * serves as buffer of all data not yet * consumed by the reader. */ + int refCount; }; + +static void +PreserveData( + TransformChannelData *dataPtr) +{ + dataPtr->refCount++; +} + +static void +ReleaseData( + TransformChannelData *dataPtr) +{ + if (--dataPtr->refCount) { + return; + } + ResultClear(&dataPtr->result); + Tcl_DecrRefCount(dataPtr->command); + ckfree((char *) dataPtr); +} /* *---------------------------------------------------------------------- @@ -240,6 +260,7 @@ TclChannelTransform( Channel *chanPtr; /* The actual channel. */ ChannelState *statePtr; /* State info for channel. */ int mode; /* Read/write mode of the channel. */ + int objc; TransformChannelData *dataPtr; Tcl_DString ds; @@ -247,6 +268,12 @@ TclChannelTransform( return TCL_ERROR; } + if (TCL_OK != Tcl_ListObjLength(interp, cmdObjPtr, &objc)) { + Tcl_SetObjResult(interp, + Tcl_NewStringObj("-command value is not a list", -1)); + return TCL_ERROR; + } + chanPtr = (Channel *) chan; statePtr = chanPtr->state; chanPtr = statePtr->topChanPtr; @@ -261,6 +288,7 @@ TclChannelTransform( dataPtr = (TransformChannelData *) ckalloc(sizeof(TransformChannelData)); + dataPtr->refCount = 1; Tcl_DStringInit(&ds); Tcl_GetChannelOption(interp, chan, "-blocking", &ds); dataPtr->readIsFlushed = 0; @@ -286,9 +314,7 @@ TclChannelTransform( if (dataPtr->self == NULL) { Tcl_AppendResult(interp, "\nfailed to stack channel \"", Tcl_GetChannelName(chan), "\"", NULL); - Tcl_DecrRefCount(dataPtr->command); - ResultClear(&dataPtr->result); - ckfree((char *) dataPtr); + ReleaseData(dataPtr); return TCL_ERROR; } @@ -296,9 +322,11 @@ TclChannelTransform( * At last initialize the transformation at the script level. */ + PreserveData(dataPtr); if ((dataPtr->mode & TCL_WRITABLE) && ExecuteCallback(dataPtr, NULL, A_CREATE_WRITE, NULL, 0, TRANSMIT_DONT, P_NO_PRESERVE) != TCL_OK){ Tcl_UnstackChannel(interp, chan); + ReleaseData(dataPtr); return TCL_ERROR; } @@ -307,9 +335,11 @@ TclChannelTransform( ExecuteCallback(dataPtr, NULL, A_DELETE_WRITE, NULL, 0, TRANSMIT_DONT, P_NO_PRESERVE); Tcl_UnstackChannel(interp, chan); + ReleaseData(dataPtr); return TCL_ERROR; } + ReleaseData(dataPtr); return TCL_OK; } @@ -350,7 +380,10 @@ ExecuteCallback( unsigned char *resBuf; Tcl_InterpState state = NULL; int res = TCL_OK; - Tcl_Obj *command = Tcl_DuplicateObj(dataPtr->command); + Tcl_Obj *command = TclListObjCopy(NULL, dataPtr->command); + Tcl_Interp *eval = dataPtr->interp; + + Tcl_Preserve(eval); /* * Step 1, create the complete command to execute. Do this by appending @@ -361,26 +394,18 @@ ExecuteCallback( */ if (preserve == P_PRESERVE) { - state = Tcl_SaveInterpState(dataPtr->interp, res); + state = Tcl_SaveInterpState(eval, res); } Tcl_IncrRefCount(command); - res = Tcl_ListObjAppendElement(dataPtr->interp, command, - Tcl_NewStringObj((char *) op, -1)); - if (res != TCL_OK) { - goto cleanup; - } + Tcl_ListObjAppendElement(NULL, command, Tcl_NewStringObj((char *) op, -1)); /* * Use a byte-array to prevent the misinterpretation of binary data coming * through as UTF while at the tcl level. */ - res = Tcl_ListObjAppendElement(dataPtr->interp, command, - Tcl_NewByteArrayObj(buf, bufLen)); - if (res != TCL_OK) { - goto cleanup; - } + Tcl_ListObjAppendElement(NULL, command, Tcl_NewByteArrayObj(buf, bufLen)); /* * Step 2, execute the command at the global level of the interpreter used @@ -390,13 +415,14 @@ ExecuteCallback( * current interpreter. Don't copy if in preservation mode. */ - res = Tcl_EvalObjEx(dataPtr->interp, command, TCL_EVAL_GLOBAL); + res = Tcl_EvalObjEx(eval, command, TCL_EVAL_GLOBAL); Tcl_DecrRefCount(command); command = NULL; - if ((res != TCL_OK) && (interp != NULL) && (dataPtr->interp != interp) + if ((res != TCL_OK) && (interp != NULL) && (eval != interp) && (preserve == P_NO_PRESERVE)) { - Tcl_SetObjResult(interp, Tcl_GetObjResult(dataPtr->interp)); + Tcl_SetObjResult(interp, Tcl_GetObjResult(eval)); + Tcl_Release(eval); return res; } @@ -411,20 +437,20 @@ ExecuteCallback( break; case TRANSMIT_DOWN: - resObj = Tcl_GetObjResult(dataPtr->interp); + resObj = Tcl_GetObjResult(eval); resBuf = Tcl_GetByteArrayFromObj(resObj, &resLen); Tcl_WriteRaw(Tcl_GetStackedChannel(dataPtr->self), (char *) resBuf, resLen); break; case TRANSMIT_SELF: - resObj = Tcl_GetObjResult(dataPtr->interp); + resObj = Tcl_GetObjResult(eval); resBuf = Tcl_GetByteArrayFromObj(resObj, &resLen); Tcl_WriteRaw(dataPtr->self, (char *) resBuf, resLen); break; case TRANSMIT_IBUF: - resObj = Tcl_GetObjResult(dataPtr->interp); + resObj = Tcl_GetObjResult(eval); resBuf = Tcl_GetByteArrayFromObj(resObj, &resLen); ResultAdd(&dataPtr->result, resBuf, resLen); break; @@ -434,24 +460,16 @@ ExecuteCallback( * Interpret result as integer number. */ - resObj = Tcl_GetObjResult(dataPtr->interp); - TclGetIntFromObj(dataPtr->interp, resObj, &dataPtr->maxRead); + resObj = Tcl_GetObjResult(eval); + TclGetIntFromObj(eval, resObj, &dataPtr->maxRead); break; } - Tcl_ResetResult(dataPtr->interp); - if (preserve == P_PRESERVE) { - (void) Tcl_RestoreInterpState(dataPtr->interp, state); - } - return res; - - cleanup: + Tcl_ResetResult(eval); if (preserve == P_PRESERVE) { - (void) Tcl_RestoreInterpState(dataPtr->interp, state); - } - if (command != NULL) { - Tcl_DecrRefCount(command); + (void) Tcl_RestoreInterpState(eval, state); } + Tcl_Release(eval); return res; } @@ -535,6 +553,7 @@ TransformCloseProc( * system rely on (f.e. signaling the close to interested parties). */ + PreserveData(dataPtr); if (dataPtr->mode & TCL_WRITABLE) { ExecuteCallback(dataPtr, interp, A_FLUSH_WRITE, NULL, 0, TRANSMIT_DOWN, P_PRESERVE); @@ -554,14 +573,13 @@ TransformCloseProc( ExecuteCallback(dataPtr, interp, A_DELETE_READ, NULL, 0, TRANSMIT_DONT, P_PRESERVE); } + ReleaseData(dataPtr); /* * General cleanup. */ - ResultClear(&dataPtr->result); - Tcl_DecrRefCount(dataPtr->command); - ckfree((char *) dataPtr); + ReleaseData(dataPtr); return TCL_OK; } @@ -606,6 +624,7 @@ TransformInputProc( gotBytes = 0; downChan = Tcl_GetStackedChannel(dataPtr->self); + PreserveData(dataPtr); while (toRead > 0) { /* * Loop until the request is satisfied (or no data is available from @@ -623,7 +642,7 @@ TransformInputProc( * break out of the loop and return to the caller. */ - return gotBytes; + break; } /* @@ -647,7 +666,7 @@ TransformInputProc( } } /* else: 'maxRead < 0' == Accept the current value of toRead. */ if (toRead <= 0) { - return gotBytes; + break; } /* @@ -663,11 +682,12 @@ TransformInputProc( */ if ((Tcl_GetErrno() == EAGAIN) && (gotBytes > 0)) { - return gotBytes; + break; } *errorCodePtr = Tcl_GetErrno(); - return -1; + gotBytes = -1; + break; } else if (read == 0) { /* * Check wether we hit on EOF in the underlying channel or not. If @@ -682,9 +702,9 @@ TransformInputProc( if (!Tcl_Eof(downChan)) { if ((gotBytes == 0) && (dataPtr->flags & CHANNEL_ASYNC)) { *errorCodePtr = EWOULDBLOCK; - return -1; + gotBytes = -1; } - return gotBytes; + break; } if (dataPtr->readIsFlushed) { @@ -692,7 +712,7 @@ TransformInputProc( * Already flushed, nothing to do anymore. */ - return gotBytes; + break; } dataPtr->readIsFlushed = 1; @@ -704,7 +724,7 @@ TransformInputProc( * We had nothing to flush. */ - return gotBytes; + break; } continue; /* at: while (toRead > 0) */ @@ -718,9 +738,11 @@ TransformInputProc( if (ExecuteCallback(dataPtr, NULL, A_READ, UCHARP(buf), read, TRANSMIT_IBUF, P_PRESERVE) != TCL_OK) { *errorCodePtr = EINVAL; - return -1; + gotBytes = -1; + break; } } /* while toRead > 0 */ + ReleaseData(dataPtr); return gotBytes; } @@ -762,11 +784,13 @@ TransformOutputProc( return 0; } + PreserveData(dataPtr); if (ExecuteCallback(dataPtr, NULL, A_WRITE, UCHARP(buf), toWrite, TRANSMIT_DOWN, P_NO_PRESERVE) != TCL_OK) { *errorCodePtr = EINVAL; - return -1; + toWrite = -1; } + ReleaseData(dataPtr); return toWrite; } @@ -819,6 +843,7 @@ TransformSeekProc( * request down, unchanged. */ + PreserveData(dataPtr); if (dataPtr->mode & TCL_WRITABLE) { ExecuteCallback(dataPtr, NULL, A_FLUSH_WRITE, NULL, 0, TRANSMIT_DOWN, P_NO_PRESERVE); @@ -830,6 +855,7 @@ TransformSeekProc( ResultClear(&dataPtr->result); dataPtr->readIsFlushed = 0; } + ReleaseData(dataPtr); return parentSeekProc(Tcl_GetChannelInstanceData(parent), offset, mode, errorCodePtr); @@ -890,6 +916,7 @@ TransformWideSeekProc( * request down, unchanged. */ + PreserveData(dataPtr); if (dataPtr->mode & TCL_WRITABLE) { ExecuteCallback(dataPtr, NULL, A_FLUSH_WRITE, NULL, 0, TRANSMIT_DOWN, P_NO_PRESERVE); @@ -901,6 +928,7 @@ TransformWideSeekProc( ResultClear(&dataPtr->result); dataPtr->readIsFlushed = 0; } + ReleaseData(dataPtr); /* * If we have a wide seek capability, we should stick with that. -- cgit v0.12 From f231f10b58b9f58583b664655b513d3f3ee0c382 Mon Sep 17 00:00:00 2001 From: sebres Date: Wed, 23 Apr 2014 09:18:20 +0000 Subject: *nix segfault cleared: we should reset a thread key after freeing of alloc cache (in tclUnixThrd.c) --- unix/tclUnixThrd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/unix/tclUnixThrd.c b/unix/tclUnixThrd.c index ad36242..d30791d 100644 --- a/unix/tclUnixThrd.c +++ b/unix/tclUnixThrd.c @@ -814,6 +814,7 @@ TclpFreeAllocCache( */ TclFreeAllocCache(ptr); + pthread_setspecific(key, NULL); } else if (initialized) { /* -- cgit v0.12 From 790e5adaf4cabf6c9dcaa3d109427dbe18f786ff Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 24 Apr 2014 15:27:58 +0000 Subject: Make sure the ReflectedChannel struct is freed in the handler thread, where it was allocated. This constraint allows the struct to safely hold Tcl_Obj values, which has been convenient for storing callback commands. --- generic/tclIORChan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/generic/tclIORChan.c b/generic/tclIORChan.c index e462f61..94428bb 100644 --- a/generic/tclIORChan.c +++ b/generic/tclIORChan.c @@ -1145,6 +1145,7 @@ ReflectClose( if (result != TCL_OK) { FreeReceivedError(&p); } + return EOK; } #endif @@ -1169,8 +1170,6 @@ ReflectClose( Tcl_DeleteEvents(ReflectEventDelete, rcPtr); - Tcl_EventuallyFree(rcPtr, (Tcl_FreeProc *) FreeReflectedChannel); - if (result != TCL_OK) { PassReceivedErrorInterp(interp, &p); } @@ -2903,6 +2902,7 @@ ForwardProc( Tcl_GetChannelName(rcPtr->chan)); Tcl_DeleteHashEntry(hPtr); + Tcl_EventuallyFree(rcPtr, (Tcl_FreeProc *) FreeReflectedChannel); break; case ForwardedInput: { -- cgit v0.12