Tcl Source Code

View Ticket
Login
Bounty program for improvements to Tcl and certain Tcl packages.
Ticket UUID: 17f747a4a4720f8e9797e7933dd43745b73e0e2c
Title: Thread safety in tclZipfs.c
Type: Bug Version: core_zip_vfs
Submitter: chw Created on: 2017-11-21 05:48:03
Subsystem: 49. Threading Assigned To: nobody
Priority: 5 Medium Severity: Critical
Status: Open Last Modified: 2017-11-21 14:24:34
Resolution: Fixed Closed By: nobody
    Closed on:
Description:
There's a static cache of Tcl_Obj pointers (e.g. "zipfs_literal_fstype") to cache
commonly used strings. This seems to be wrong since it potentially is used by
more than a single thread and lacks synchronization.

Either must that cache be left out or implemented using thread local storage as
in the Tcl_SetStartupScript() function.
User Comments: hypnotoad added on 2017-11-21 14:24:34:
I removed the pool of Tcl_Objs, and replaced with a single pointer to a constant string once the discovery process has run. Functions that return a Tcl_Obj run a fresh Tcl_NewStringObj every time against the constant string.

Checkin: [9f0abfb1ac]

sebres added on 2017-11-21 14:17:40:
Sharing of Tcl_Obj between different threads is a very bad idea (and not allowed in standard tcl-core).
And this is not necessarily a thing of atomic refCount incr/decr (resp. object modifications) - default tcl-core has neither a GIL, nor a multiple internal representation.

This meant: one thread can make an integer from the object, another makes unicode string or list (with another internal representation).
Meaning the object changes internally and this will produce a BOOM (segfaults and co.).

Don't do such kind of sharing. Therefore:

- either move the object completely to another thread (take over the ownership);
- or make a duplicate (using Tcl_DuplicateObj) in order to supply it to different threads;
- or create each time a new object by the command exec as suggested from @chw.

chw added on 2017-11-21 13:45:47:
Maybe I'm missing something here, but the decrement in 

 #define Tcl_DecrRefCount(objPtr) \
   do { \
     Tcl_Obj *_objPtr = (objPtr); \
     if ((_objPtr)->refCount-- <= 1) { \
       TclFreeObj(_objPtr); \
   } \
 } while(0)

is not guaranteed to be atomic. No gcc/clang trickery,
no Microsoften InterlockedDecrement()ery.

Thus, yes, please Tcl_NewString() from constant strings.

hypnotoad added on 2017-11-21 12:07:31:
It might also be worth a quick read of tclLiteral.c to see how things are done in there.

hypnotoad added on 2017-11-21 12:05:46:
There was a lively discussion on stackoverflow on the subject:

https://stackoverflow.com/questions/28462414/alloc-invalid-block-are-tcl-incrrefcount-and-tcl-decrrefcount-thread-safe-for

The long and short answer is that Tcl_IncrRefCount is perfectly Thread safe. Passing values around between threads... no to much. Mostly to do with shimmering. 

On one hand these are marked as shared objects, and anyone who was going to modify them should clone them and modify a copy. On the other... if someone does manage to modify them it will be hell to figure out what is going on.

Let's meet halfway. Instead of storing Tcl_Objs, let's store constant strings. And all of the functions that return Tcl_Objs will produces a fresh copy of those strings.

chw added on 2017-11-21 11:30:46:
I can't demonstrate it, however, ask if Tcl_DecrRefCount() and Tcl_IncrRefCount()
are thread safe. AFAIK, they are not. And this might be the real underlying problem.

hypnotoad added on 2017-11-21 10:35:10:
Considering these are constants that are only generated once in a process lifetime, nothing should be changing them once set. Tcl_SetStartupScript needs to be thread aware because the startup script for a thread could very well be different than for the main process.

Can you demonstrate this actually causing a problem?