Tcl Source Code

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

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

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.

@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.

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.

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.

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

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

Sorry, Ashok.

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??

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?

Here's a more complete log of my steps to see the failures:

$ uname -a
Linux spalt.cam.nist.gov 5.15.0-131-generic #141-Ubuntu SMP Fri Jan 10 21:18:28 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux

$ mkdir demo-87b697
$ cd demo-87b697/
$ fossil open ../tcl.fr 0433b6
...
$ cd unix/
$ ./configure --disable-shared
...
checking system version... (cached) Linux-5.15.0-131-generic
...
checking for macher... checking for zip... /usr/bin/zip
Found INFO Zip in environment
checking for building with zipfs... yes
...

$ make
...
$ make tcltest
...
$ make test-tcl TESTFLAGS="-file encoding.test"
...
LD_LIBRARY_PATH=`pwd`:/home/dgp/lib TCLLIBPATH="/home/dgp/fossil/demo-87b697/unix/pkgs" TCL_LIBRARY="" ./tcltest /home/dgp/fossil/demo-87b697/tests/all.tcl -file encoding.test
Tests running in interp:  /local/tmp/dgp/fossil/demo-87b697/unix/tcltest
Tests located in:  /local/tmp/dgp/fossil/demo-87b697/tests
Tests running in:  /local/tmp/dgp/fossil/demo-87b697/unix
...
encoding.test


==== encoding-28.0 all encodings load FAILED
==== Contents of test case:

	set string hello
	foreach name [encoding names] {
		if {$name ne "unicode"} {
		    incr count
		}
		encoding convertto -profile tcl8 $name $string

		# discard the cached internal representation of Tcl_Encoding
		# Unfortunately, without this, encoding 2-1 fails.
		llength $name
	}
	return $count

---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: invalid encoding file "tis-620"
    while executing
"encoding convertto -profile tcl8 $name $string"
    ("foreach" body line 5)
    invoked from within
"foreach name [encoding names] {
		if {$name ne "unicode"} {
		    incr count
		}
		encoding convertto -profile tcl8 $name $string

		# discard the cac..."
    ("uplevel" body line 3)
    invoked from within
"uplevel 1 $script"
---- errorCode: TCL LOOKUP ENCODING tis-620
==== encoding-28.0 FAILED



==== encoding-12.7 cp864 [ecafd8611d] FAILED
==== Contents of test case:

    encoding convertfrom cp864 \xA7

---- Test generated error; Return code was: 1
---- Return code should have been one of: 0 2
---- errorInfo: unexpected byte sequence starting at index 0: '\xA7'
    while executing
"encoding convertfrom cp864 \xA7"
    ("uplevel" body line 2)
    invoked from within
"uplevel 1 $script"
---- errorCode: TCL ENCODING ILLEGALSEQUENCE 0
==== encoding-12.7 FAILED

...

@dgp, does [9338fcde504fb307] changes something in the tests, Don? (I mean if Tcl_GetEncoding fails for whatever reason, and the error would not be silently swallowed now, so could now bring more insides).

By the way, like Ashok, I can't reproduce it on my linux boxes either.

The checkin now on the tip of the core-9-0-branch doesn't change
this failure.  No opinion whether or not to keep it.

When I see the failing tests, I am picking up a set of encoding files
from a very old install, sometime around 2018.

Relative to where I make most Tcl installs, I see

  $installDir/lib/tcl9.0/encoding

and the contents of that directory are files placed there in 2018.

That's even earlier than the release of Tcl 9.0a1, so it's some litter
from pre-release development testing.  I expect that if I clean up my
disk drive the symptoms will disappear.

Under the Tcl 8 patterns of development, those old files would have
been overwritten by corrected/updated versions in installs of later
Tcl 9.0 releases.  But Tcl 9.0 has shifted to placing these files
in the zip archive, so such installs don't happen anymore.

Starting with [0433b67adc], `make test` is finding and using this litter,
when it hasn't done so recently before that.  The checkin somehow started
looking there and using what is found there.  If there's a sound reason
for that, let's understand it.  If not, lets find and destroy searches
that might find what can only be wrong answers.

Is this the place where the *.enc files get installed after
a --disable-zipfs build??

Why would my --disable-shared --enable-zipfs build be 
searching there?

Looks like a weird side effect to me.

What would you see with this one (started from build folder):

LD_LIBRARY_PATH=. TCL_LIBRARY="" ./tcltest <<<'puts [encoding dirs]'

In particular do you see some other path before "//zipfs:/app/tcl_library/encoding" (for zipfs) or "${pwd}/../library/encoding" (for build without zipfs)?

Something like:

/usr/local/lib/tcl9.0/encoding ${pwd}/../library/encoding

(or whatever the $installDir you mentioned)?

Strange is, even if I create such directory (and put there some old encodings, e. g. from core-9-0-a1-rc), I cannot reproduce the errors in "encoding.test" anyway (no matter with or without zipfs, disabled shared or not). And I don't know what shall be so different there to force that tests fail.

Moreover in build with zipfs, I see only one path in `encoding dirs` - "//zipfs:/app/tcl_library/encoding" (and since zipfs uses `Tcl_SetEncodingSearchPath` to set search path, it shall be basically sole path only).

By the way, does this one returns 1?

LD_LIBRARY_PATH=. ./tcltest <<<'puts [file exists "//zipfs:/app/tcl_library/encoding"]'

OK, ultimately found that - [9ade301dd7122d21] shall fix it now.

It avoids too earlier search for tcl-library - previously TclZipfsLocateTclLibrary has been invoked before mount of zipfs, so obviously can't find something at time point if TclZipfs_AppHook got called, and since it is called once (and not as before [0433b67adc] for any interpreter), it wouldn't initialize properly (misses a lot of inits, inclusive invocation of TclZipfsInitEncodingDirs).

As I mentioned on the online meet, I had the feeling Don having reported this with an earlier commit as well. Found it way back in 2023 - https://core.tcl-lang.org/tcl/tktview/894e11d7f73023089a8f. Though on the core-8-branch, essentially the same issue with outdated encodings being found. Not that it matters, but I think this issue has existed for some time and triggers under some specific circumstances.

FWIW, I really do not like Tcl's propensity of "searching" for file locations, whether at build time (headers, libraries), during test, or runtime (scripts, encodings). The current error is an example but not the only one. Many users also run into this issue, finding the wrong headers or libraries while trying to build Tcl.


IMO Tcl should never be searching and randomly picking up something that *might* be the right file(s). Look in a fixed location or if not found there have the user specify the location via a configure option, environment variable etc.

> Sergey, that change seems to break --enable-shared builds

That - indeed - broke the build. So I moved this commit to Ashok's "apn-missing-shlib-zipfs-check" branch (which also contains a fix for this breakage).

@Sergey, please don't commit directly to core-9-0-branch/trunk unless you are 1000% sure it won't break anything ....

@Ashok, as soon as your fixes are complete, merging it to core-9-0-branch and trunk should be possible without problems.

@Don, it has been bothering me as to why this issue is exhibited only on your system. I think I have a plausible explanation. It was a combination of 

- the bug introduced by my commit (that Sergey fixed) related to zipfs initialization in static build case, and

- what I would suggest is a misfeature (by design) in the initialization script's use of `pkgconfig scriptdir,runtime`

The init script constructs a list of directories to search for the init.tcl file. One of these is the directory returned by `pkgconfig get scriptdir,runtime`. Because the zipfs failed to locate tcl_library early enough, and the `TCL_LIBRARY` env is not set (by design when running make test with zipfs), the test run lands up using the path returned by `pkgconfig` command. The problem is that the `scriptdir,runtime` value reflects the path **after installation** (presumably the value of `-prefix` or friends). When you ran test-tcl, it was picking up the path where the build would be installed and that path happened to contain the old encodings.

That's my **theory** anyways.

That pkgconfig value is pretty useless as on Windows it never reflects reality and is broken in various scenarios even on Unix, like the one above.

Closing. @dgp, please if you are still experiencing issues, please re-open or log a separate ticket.

I can confirm that the tests no longer fail on the
current tip of the core-9-0-branch.  Thanks!

From the comments it looks like there is a remaining
bug, which I will open in a new ticket.