Tcl Source Code

What is the test configuration in which you are seeing errors? Neither my tests not Github CI showed any failures on any platform before I committed.

On the current core-9-0-branch, Github CI still shows no failures and now I re-tested the following combinations on Ubuntu:

```
configure
make test
make test TESTFLAGS="-singleproc 1"

configure --disable-shared
make test
make test TESTFLAGS="-singleproc 1"

configure --disable-zipfs
make test
make test TESTFLAGS="-singleproc 1"
```

as well as the equivalents on Windows

```
nmake /f makefile.vc test
nmake /f makefile.vc test TESTFLAGS="-singleproc 1"
nmake /f makefile.vc OPTS=static test
nmake /f makefile.vc OPTS=static test TESTFLAGS="-singleproc 1"
nmake /f makefile.vc OPTS=noembed test
nmake /f makefile.vc OPTS=noembed test TESTFLAGS="-singleproc 1"

```

None show any failures.

Failing on ASCII input 0x7f is very strange indeed and if it seems to happen only on your system stranger still. Something uninitialized in specific circumstance...?

What platform are you on and what were the specific configure and test options?

On core-9-0-branch, starting with checkin [0433b67adc], I see 114
failing tests in cmdAH.test and encoding.test.  The first of them is


==== cmdAH-4.3.13.7F.solo.ascii.strict.a encoding convertfrom -profile strict ascii  FAILED
==== Contents of test case:
encoding convertfrom -profile strict ascii 
---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: unexpected byte sequence starting at index 0: '\x7F'
    while executing
"encoding convertfrom -profile strict ascii "
    ("uplevel" body line 1)
    invoked from within
"uplevel 1 $script"
---- errorCode: TCL ENCODING ILLEGALSEQUENCE 0
==== cmdAH-4.3.13.7F.solo.ascii.strict.a FAILED


How can I help unbreak the branch??

Review remark: indenting in tclEncoding.c is not consistent. All other files in this commit are fine. Fixed [ee22d2717fc9d6c3|here].

Sorry, Ashok.

Fixed in [0433b67adc]. Broader thread safety issues with systemEncoding to be addressed separately.

Yes of course. But tracking that as a separate bug that you have already logged (and I duplicated yesterday) as the required solution is much broader (as you also stated). The possibility of race conditions in encoding routines is much greater and has to be fixed.

No problem.

> to convince oneself there are no race conditions or other effects

But there is a race condition issue. The possibility to overwrite a system encoding from every thread (also protected by mutex) will cause that previously set system-encoding, which can be still used by other threads with other functions (see ticket [f2ff05fc84]) may get released (by last reference). And your extra mutex doesn't protect against it at all. Sure, it makes that not more worse as it was, but the lock protection is definitely overestimated...
As already said, it is more or less conceptual bug in Tcl_UtfToExternal*, Tcl_ExternalToUtf* and co.

@sebres, I'm merging my original code. What you said about not requiring a lock may well be true but except in performance critical paths (this is not one), I prefer to stick with the lock for shared data in the general belief that ensuring access is safe without locks needs careful thought and is prone to errors (possibly even in future when code is modified). Not worth the time spent, imo, to convince oneself there are no race conditions or other effects.

After a quick review, I don't entirely agree with the changes in Tcl_SetSystemEncoding, because it may have an issue by moving of Tcl_MutexLock(&encodingMutex) to head of function, which now would cause that due to Tcl_GetEncoding() (which also locks mutex) would do that twice (enters the lock recursively).
Note that the mutexes are not necessarily reentrant on all systems, so a recursive locking may be an UB (and may cause a deadlock). Also Tcls documentation says that "the result of locking a mutex twice from the same thread is undefined".

Additionally note that this mutex only protect the hash table of encodings and encoding' refCount, it doesn't really protect static systemEncoding (nor the defaultEncoding) directly. Let alone it'd be still affected by [f2ff05fc84] and co.

Commit [bf62ce24c60c7b8a] shall fix it now.

Reminder to self - need to investigate if wish / Tk have a similar issue.

Looks like the bug was introduced with commit [450d12dac5].