From d25403a5a8b4f784566eb15f30b46ed568efc478 Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 15 Apr 2014 16:09:28 +0000 Subject: [88aef05cda] Stop reentrancy segfault in reflected channels by managing callbacks as (copies of) lists, not shared Tcl_Obj arrays. Still could use cleanup and improvements. --- generic/tclIORChan.c | 22 ++++++++++++++++++++-- tests/ioCmd.test | 19 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/generic/tclIORChan.c b/generic/tclIORChan.c index affed02..2d712a2 100644 --- a/generic/tclIORChan.c +++ b/generic/tclIORChan.c @@ -110,6 +110,7 @@ typedef struct { * plus 4 placeholders for method, channel, * and at most two varying (method specific) * words. */ + Tcl_Obj *cmd; /* */ int methods; /* Bitmask of supported methods */ /* @@ -2044,6 +2045,10 @@ NewReflectedChannel( */ /* ASSERT: cmdpfxObj is a Tcl List */ + rcPtr->cmd = TclListObjCopy(NULL, cmdpfxObj); + Tcl_ListObjAppendElement(NULL, rcPtr->cmd, Tcl_NewObj()); + Tcl_ListObjAppendElement(NULL, rcPtr->cmd, handleObj); + Tcl_IncrRefCount(rcPtr->cmd); Tcl_ListObjGetElements(interp, cmdpfxObj, &listc, &listv); @@ -2158,6 +2163,7 @@ FreeReflectedChannel( */ Tcl_DecrRefCount(rcPtr->argv[n+1]); + Tcl_DecrRefCount(rcPtr->cmd); ckfree((char*) rcPtr->argv); ckfree((char*) rcPtr); @@ -2200,6 +2206,8 @@ InvokeTclMethod( Tcl_InterpState sr; /* State of handler interp */ int result; /* Result code of method invokation */ Tcl_Obj *resObj = NULL; /* Result of method invokation. */ + Tcl_Obj *cmd; + int len; if (!rcPtr->interp) { /* @@ -2236,6 +2244,11 @@ InvokeTclMethod( Tcl_IncrRefCount(methObj); rcPtr->argv[rcPtr->argc - 2] = methObj; + cmd = TclListObjCopy(NULL, rcPtr->cmd); + ListObjLength(cmd, len); + Tcl_ListObjReplace(NULL, cmd, len - 2, 1, 1, &methObj); + + /* * Append the additional argument containing method specific details * behind the channel id. If specified. @@ -2244,9 +2257,11 @@ InvokeTclMethod( cmdc = rcPtr->argc; if (argOneObj) { rcPtr->argv[cmdc] = argOneObj; + Tcl_ListObjAppendElement(NULL, cmd, argOneObj); cmdc++; if (argTwoObj) { rcPtr->argv[cmdc] = argTwoObj; + Tcl_ListObjAppendElement(NULL, cmd, argTwoObj); cmdc++; } } @@ -2256,9 +2271,11 @@ InvokeTclMethod( * existing state intact. */ + Tcl_IncrRefCount(cmd); sr = Tcl_SaveInterpState(rcPtr->interp, 0 /* Dummy */); Tcl_Preserve(rcPtr->interp); - result = Tcl_EvalObjv(rcPtr->interp, cmdc, rcPtr->argv, TCL_EVAL_GLOBAL); +// result = Tcl_EvalObjv(rcPtr->interp, cmdc, rcPtr->argv, TCL_EVAL_GLOBAL); + result = Tcl_GlobalEvalObj(rcPtr->interp, cmd); /* * We do not try to extract the result information if the caller has no @@ -2284,7 +2301,7 @@ InvokeTclMethod( */ if (result != TCL_ERROR) { - Tcl_Obj *cmd = Tcl_NewListObj(cmdc, rcPtr->argv); +// Tcl_Obj *cmd = Tcl_NewListObj(cmdc, rcPtr->argv); int cmdLen; const char *cmdString = Tcl_GetStringFromObj(cmd, &cmdLen); @@ -2303,6 +2320,7 @@ InvokeTclMethod( } Tcl_IncrRefCount(resObj); } + Tcl_DecrRefCount(cmd); Tcl_RestoreInterpState(rcPtr->interp, sr); Tcl_Release(rcPtr->interp); diff --git a/tests/ioCmd.test b/tests/ioCmd.test index bdf0fb3..f021ade 100644 --- a/tests/ioCmd.test +++ b/tests/ioCmd.test @@ -755,6 +755,25 @@ test iocmd-21.19 {chan create, init failure -> no channel, no finalize} -match g rename foo {} set res } -result {{} {initialize rc* {read write}} 1 {*all required methods*} {}} +test iocmd-21.20 {Bug 88aef05cda} -setup { + proc foo {method chan args} { + switch -- $method blocking { + chan configure $chan -blocking [lindex $args 0] + return + } initialize { + return {initialize finalize watch blocking read write + configure cget cgetall} + } finalize { + return + } + } + set ch [chan create {read write} foo] +} -body { + list [catch {chan configure $ch -blocking 0} m] $m +} -cleanup { + close $ch + rename foo {} +} -match glob -result {1 {*nested eval*}} # --- --- --- --------- --------- --------- # Helper commands to record the arguments to handler methods. -- cgit v0.12 From 630c5dd08faa0ca5deac0606be3ac1b36b82a6d3 Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 15 Apr 2014 17:45:26 +0000 Subject: Purge (now unused) argc and argv fields. --- generic/tclIORChan.c | 110 ++------------------------------------------------- 1 file changed, 3 insertions(+), 107 deletions(-) diff --git a/generic/tclIORChan.c b/generic/tclIORChan.c index 2d712a2..eaabdfb 100644 --- a/generic/tclIORChan.c +++ b/generic/tclIORChan.c @@ -91,26 +91,7 @@ typedef struct { #ifdef TCL_THREADS Tcl_ThreadId thread; /* Thread the 'interp' belongs to. */ #endif - - /* See [==] as well. - * Storage for the command prefix and the additional words required for - * the invocation of methods in the command handler. - * - * argv [0] ... [.] | [argc-2] [argc-1] | [argc] [argc+2] - * cmd ... pfx | method chan | detail1 detail2 - * ~~~~ CT ~~~ ~~ CT ~~ - * - * CT = Belongs to the 'Command handler Thread'. - */ - - int argc; /* Number of preallocated words - 2 */ - Tcl_Obj **argv; /* Preallocated array for calling the handler. - * args[0] is placeholder for cmd word. - * Followed by the arguments in the prefix, - * plus 4 placeholders for method, channel, - * and at most two varying (method specific) - * words. */ - Tcl_Obj *cmd; /* */ + Tcl_Obj *cmd; /* Callback command prefix */ int methods; /* Bitmask of supported methods */ /* @@ -2023,8 +2004,6 @@ NewReflectedChannel( Tcl_Obj *handleObj) { ReflectedChannel *rcPtr; - int i, listc; - Tcl_Obj **listv; rcPtr = (ReflectedChannel *) ckalloc(sizeof(ReflectedChannel)); @@ -2040,58 +2019,11 @@ NewReflectedChannel( rcPtr->mode = mode; rcPtr->interest = 0; /* Initially no interest registered */ - /* - * Method placeholder. - */ - /* ASSERT: cmdpfxObj is a Tcl List */ rcPtr->cmd = TclListObjCopy(NULL, cmdpfxObj); Tcl_ListObjAppendElement(NULL, rcPtr->cmd, Tcl_NewObj()); Tcl_ListObjAppendElement(NULL, rcPtr->cmd, handleObj); Tcl_IncrRefCount(rcPtr->cmd); - - Tcl_ListObjGetElements(interp, cmdpfxObj, &listc, &listv); - - /* - * See [==] as well. - * Storage for the command prefix and the additional words required for - * the invocation of methods in the command handler. - * - * listv [0] [listc-1] | [listc] [listc+1] | - * argv [0] ... [.] | [argc-2] [argc-1] | [argc] [argc+2] - * cmd ... pfx | method chan | detail1 detail2 - */ - - rcPtr->argc = listc + 2; - rcPtr->argv = (Tcl_Obj **) ckalloc(sizeof(Tcl_Obj *) * (listc+4)); - - /* - * Duplicate object references. - */ - - for (i=0; iargv[i] = listv[i]; - - Tcl_IncrRefCount(word); - } - - i++; /* Skip placeholder for method */ - - /* - * [Bug 1667990]: See [x] in FreeReflectedChannel for release - */ - - rcPtr->argv[i] = handleObj; - Tcl_IncrRefCount(handleObj); - - /* - * The next two objects are kept empty, varying arguments. - */ - - /* - * Initialization complete. - */ - return rcPtr; } @@ -2142,7 +2074,6 @@ FreeReflectedChannel( ReflectedChannel *rcPtr) { Channel *chanPtr = (Channel *) rcPtr->chan; - int i, n; if (chanPtr->typePtr != &tclRChannelType) { /* @@ -2152,20 +2083,7 @@ FreeReflectedChannel( ckfree((char*) chanPtr->typePtr); } Tcl_Release(chanPtr); - - n = rcPtr->argc - 2; - for (i=0; iargv[i]); - } - - /* - * [Bug 1667990]: See [x] in NewReflectedChannel for lock. n+1 = argc-1. - */ - - Tcl_DecrRefCount(rcPtr->argv[n+1]); Tcl_DecrRefCount(rcPtr->cmd); - - ckfree((char*) rcPtr->argv); ckfree((char*) rcPtr); } @@ -2201,7 +2119,6 @@ InvokeTclMethod( Tcl_Obj *argTwoObj, /* NULL'able */ Tcl_Obj **resultObjPtr) /* NULL'able */ { - int cmdc; /* #words in constructed command */ Tcl_Obj *methObj = NULL; /* Method name in object form */ Tcl_InterpState sr; /* State of handler interp */ int result; /* Result code of method invokation */ @@ -2236,33 +2153,24 @@ InvokeTclMethod( */ /* - * Insert method into the pre-allocated area, after the command prefix, + * Insert method into the callback command, after the command prefix, * before the channel id. */ methObj = Tcl_NewStringObj(method, -1); - Tcl_IncrRefCount(methObj); - rcPtr->argv[rcPtr->argc - 2] = methObj; - cmd = TclListObjCopy(NULL, rcPtr->cmd); ListObjLength(cmd, len); Tcl_ListObjReplace(NULL, cmd, len - 2, 1, 1, &methObj); - /* * Append the additional argument containing method specific details * behind the channel id. If specified. */ - cmdc = rcPtr->argc; if (argOneObj) { - rcPtr->argv[cmdc] = argOneObj; Tcl_ListObjAppendElement(NULL, cmd, argOneObj); - cmdc++; if (argTwoObj) { - rcPtr->argv[cmdc] = argTwoObj; Tcl_ListObjAppendElement(NULL, cmd, argTwoObj); - cmdc++; } } @@ -2274,7 +2182,6 @@ InvokeTclMethod( Tcl_IncrRefCount(cmd); sr = Tcl_SaveInterpState(rcPtr->interp, 0 /* Dummy */); Tcl_Preserve(rcPtr->interp); -// result = Tcl_EvalObjv(rcPtr->interp, cmdc, rcPtr->argv, TCL_EVAL_GLOBAL); result = Tcl_GlobalEvalObj(rcPtr->interp, cmd); /* @@ -2301,7 +2208,6 @@ InvokeTclMethod( */ if (result != TCL_ERROR) { -// Tcl_Obj *cmd = Tcl_NewListObj(cmdc, rcPtr->argv); int cmdLen; const char *cmdString = Tcl_GetStringFromObj(cmd, &cmdLen); @@ -2325,16 +2231,6 @@ InvokeTclMethod( Tcl_Release(rcPtr->interp); /* - * Cleanup of the dynamic parts of the command. - * - * The detail objects survived the Tcl_EvalObjv without change because of - * the contract. Therefore there is no need to decrement the refcounts. Only - * the internal method object has to be disposed of. - */ - - Tcl_DecrRefCount(methObj); - - /* * The resObj has a ref count of 1 at this location. This means that the * caller of InvokeTclMethod has to dispose of it (but only if it was * returned to it). @@ -2857,7 +2753,7 @@ ForwardProc( } /* - * Freeing is done here, in the origin thread, because the argv[] + * Freeing is done here, in the origin thread, callback command * objects belong to this thread. Deallocating them in a different * thread is not allowed * -- cgit v0.12 From 551367d26fffabaf2176de988521a98184f49ad0 Mon Sep 17 00:00:00 2001 From: "jan.nijtmans" Date: Wed, 16 Apr 2014 09:28:54 +0000 Subject: Upgrade from Winsock 1.1 to Winsock 2.2, which is always available on Win2000+. See: [http://msdn.microsoft.com/en-us/library/windows/desktop/ms742213%28v=vs.85%29.aspx] for details. Move winsock initialization to TclpInitPlatform(void), so we can be sure everywhere that we have an initialized winsock2. Stub entries for TclWinGetServByName/TclWinGetSockOpt/TclWinSetSockOpt are no longer necessary (will be removed in 9.0, but are kept in 8.x) --- generic/tclIntPlatDecls.h | 6 ++++ win/tclWinInit.c | 24 ++++++--------- win/tclWinPort.h | 9 ------ win/tclWinSock.c | 78 ++--------------------------------------------- 4 files changed, 19 insertions(+), 98 deletions(-) diff --git a/generic/tclIntPlatDecls.h b/generic/tclIntPlatDecls.h index 80dd2ad..f935efb 100644 --- a/generic/tclIntPlatDecls.h +++ b/generic/tclIntPlatDecls.h @@ -849,7 +849,13 @@ extern TclIntPlatStubs *tclIntPlatStubsPtr; #if defined(__WIN32__) || defined(__CYGWIN__) # undef TclWinNToHS +# undef TclWinGetServByName +# undef TclWinGetSockOpt +# undef TclWinSetSockOpt # define TclWinNToHS ntohs +# define TclWinGetServByName getservbyname +# define TclWinGetSockOpt getsockopt +# define TclWinSetSockOpt setsockopt #else # undef TclpGetPid # define TclpGetPid(pid) ((unsigned long) (pid)) diff --git a/win/tclWinInit.c b/win/tclWinInit.c index 2f3c7e8..4e860b2 100644 --- a/win/tclWinInit.c +++ b/win/tclWinInit.c @@ -113,8 +113,8 @@ static int ToUtf(CONST WCHAR *wSrc, char *dst); * * TclpInitPlatform -- * - * Initialize all the platform-dependant things like signals and - * floating-point error handling. + * Initialize all the platform-dependant things like signals, + * floating-point error handling and sockets. * * Called at process initialization time. * @@ -130,20 +130,16 @@ static int ToUtf(CONST WCHAR *wSrc, char *dst); void TclpInitPlatform(void) { - tclPlatform = TCL_PLATFORM_WINDOWS; + WSADATA wsaData; + WORD wVersionRequested = MAKEWORD(2, 2); - /* - * The following code stops Windows 3.X and Windows NT 3.51 from - * automatically putting up Sharing Violation dialogs, e.g, when someone - * tries to access a file that is locked or a drive with no disk in it. - * Tcl already returns the appropriate error to the caller, and they can - * decide to put up their own dialog in response to that failure. - * - * Under 95 and NT 4.0, this is a NOOP because the system doesn't - * automatically put up dialogs when the above operations fail. - */ + tclPlatform = TCL_PLATFORM_WINDOWS; - SetErrorMode(SetErrorMode(0) | SEM_FAILCRITICALERRORS); + /* + * Initialize the winsock library. On Windows XP and higher this + * can never fail. + */ + WSAStartup(wVersionRequested, &wsaData); #ifdef STATIC_BUILD /* diff --git a/win/tclWinPort.h b/win/tclWinPort.h index ec9e867..ea6d8f8 100644 --- a/win/tclWinPort.h +++ b/win/tclWinPort.h @@ -448,15 +448,6 @@ typedef DWORD_PTR * PDWORD_PTR; #define TclpSysRealloc(ptr, size) ((void*)HeapReAlloc(GetProcessHeap(), \ (DWORD)0, (LPVOID)ptr, (DWORD)size)) -/* - * The following defines map from standard socket names to our internal - * wrappers that redirect through the winSock function table (see the - * file tclWinSock.c). - */ - -#define getservbyname TclWinGetServByName -#define getsockopt TclWinGetSockOpt -#define setsockopt TclWinSetSockOpt /* This type is not defined in the Windows headers */ #define socklen_t int diff --git a/win/tclWinSock.c b/win/tclWinSock.c index aa298f5..e18a3dd 100644 --- a/win/tclWinSock.c +++ b/win/tclWinSock.c @@ -263,8 +263,6 @@ static void InitSockets(void) { DWORD id; - WSADATA wsaData; - DWORD err; ThreadSpecificData *tsdPtr = (ThreadSpecificData *) TclThreadDataKeyGet(&dataKey); @@ -295,38 +293,6 @@ InitSockets(void) goto initFailure; } - /* - * Initialize the winsock library and check the interface version - * actually loaded. We only ask for the 1.1 interface and do require - * that it not be less than 1.1. - */ - -#define WSA_VERSION_MAJOR 1 -#define WSA_VERSION_MINOR 1 -#define WSA_VERSION_REQD MAKEWORD(WSA_VERSION_MAJOR, WSA_VERSION_MINOR) - - err = WSAStartup((WORD)WSA_VERSION_REQD, &wsaData); - if (err != 0) { - TclWinConvertWSAError(err); - goto initFailure; - } - - /* - * Note the byte positions are swapped for the comparison, so that - * 0x0002 (2.0, MAKEWORD(2,0)) doesn't look less than 0x0101 (1.1). - * We want the comparison to be 0x0200 < 0x0101. - */ - - if (MAKEWORD(HIBYTE(wsaData.wVersion), LOBYTE(wsaData.wVersion)) - < MAKEWORD(WSA_VERSION_MINOR, WSA_VERSION_MAJOR)) { - TclWinConvertWSAError(WSAVERNOTSUPPORTED); - WSACleanup(); - goto initFailure; - } - -#undef WSA_VERSION_REQD -#undef WSA_VERSION_MAJOR -#undef WSA_VERSION_MINOR } /* @@ -434,7 +400,6 @@ SocketExitHandler( TclpFinalizeSockets(); UnregisterClass("TclSocket", TclWinGetTclInstance()); - WSACleanup(); initialized = 0; Tcl_MutexUnlock(&socketMutex); } @@ -2564,71 +2529,34 @@ InitializeHostName( *---------------------------------------------------------------------- */ +#undef TclWinGetSockOpt int TclWinGetSockOpt(SOCKET s, int level, int optname, char *optval, int *optlen) { - /* - * Check that WinSock is initialized; do not call it if not, to prevent - * system crashes. This can happen at exit time if the exit handler for - * WinSock ran before other exit handlers that want to use sockets. - */ - - if (!SocketsEnabled()) { - return SOCKET_ERROR; - } - return getsockopt(s, level, optname, optval, optlen); } +#undef TclWinSetSockOpt int TclWinSetSockOpt(SOCKET s, int level, int optname, const char *optval, int optlen) { - /* - * Check that WinSock is initialized; do not call it if not, to prevent - * system crashes. This can happen at exit time if the exit handler for - * WinSock ran before other exit handlers that want to use sockets. - */ - - if (!SocketsEnabled()) { - return SOCKET_ERROR; - } - return setsockopt(s, level, optname, optval, optlen); } char * TclpInetNtoa(struct in_addr addr) { - /* - * Check that WinSock is initialized; do not call it if not, to prevent - * system crashes. This can happen at exit time if the exit handler for - * WinSock ran before other exit handlers that want to use sockets. - */ - - if (!SocketsEnabled()) { - return NULL; - } - return inet_ntoa(addr); } +#undef TclWinGetServByName struct servent * TclWinGetServByName( const char *name, const char *proto) { - /* - * Check that WinSock is initialized; do not call it if not, to prevent - * system crashes. This can happen at exit time if the exit handler for - * WinSock ran before other exit handlers that want to use sockets. - */ - - if (!SocketsEnabled()) { - return NULL; - } - return getservbyname(name, proto); } -- cgit v0.12