tcl-core

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[Jan N. <jan...@gm...>]

Op wo 15 jan 2025 om 11:43 schreef Ashok:
> I do wish Jan, assuming he has read the references I posted previously, would address at least the two points I specifically asked about in my post. To quote,
>
> Per my reading of just the TLS-related portions,
> •            It does not allocate slots for the *implicit* TLS entries and initialize them in the loading thread.
> •            It does not update the TLS map for existing threads by reallocating (if necessary) the TLS storage and updating the in-use slot map.

See below. I never used those TLS features, and I doubt any existing
Tcl extension
uses it. Can you supply (or point to) a little bit of example code?

> If the intent is to have memory loading work for a restricted set of extensions (say no exception handling or implicit TLS) because that is useful enough to accept current risk and future risk in case of Windows changes, folks can by all means vote yes. Please do add test cases though to avoid a repetition of the zipfs situation before 9.0.

Regarding exception handling, I can be very short: the Tcl core
doesn't handle (C++) exception handling, if it is thrown from a dll
or from anywhere else. Any exception thrown internally in a dll, must
be caught within the same dll, and translated
to TCL_OK or TCL_ERROR with an error-message. The throw/catch handling
is fully within the loaded dll, which
is handled by Pull request #91. So, it should work in MemoryModule.
Two questions:
1) Can you supply a piece of test-code you suspect that won't work?
2) Can you point to any Tcl extension dll in the field using this?

Regarding TLS, same question:
1) Can you supply a piece of test-code you suspect that won't work?
2) Can you point to any Tcl extension dll in the field using this?

The future risk in Windows Changes: I don't see that happen. The
changes in Vista were
probably either for security reasons, or for extension to x64, both
are being used a long time
now in various Windows versions. I don't see the need for more changes
coming. Next
change in the Windows kernel is probably the switch to the Linux kernel.

The LUCK framework supports the following (more than 150!) dll's being loaded
by MemoryModule:
---------------------------------------------------------------------------
 app-sdx2.0  augeas0.5.0  autoopts0  awthemes10  azure-ttk  bcrypt2.0
blt2.4  bonjour1.1  BSD1.9.2  bwidget1.10  bz20.1  calc0  can2svg0.3
Canvas3d1.2.4  ceptcl0.4  classview0.1  dbus  DiffUtil0.4.2  dmtx0.7.5
 espeak0.2  Ffidl0.7  flexmenu1  forest-ttk  fsdialog1.15
fswatch2.0.1  fuse1.1  gridplus2.11  helpviewer3.0.2  icons2
Img1.4.11  imgjp20.1  itcl4.2.0  itk4.1.0  iwidgets4.1  kafka2.4
legacy_3dcanvas  lmdb0.4.3  lzma0.1  materialicons0.2  Memchan2.4
mkappimg  modbus0.1  Mpexpr12  mqtt2.0  mqtt3.1  msgpack2  nats3
notebook2.2  nsf2.4.0  ooxml1  parse_args0.5.1  parser1.8  pdf4tcl09
pdf4tcl_graph1.0  photoframe1  piio1.3  pikchr1.0  promise1
pty_tcl0.1  puppyicons0.1  ral0.12.2  ralutil0.12.2  retcl0  rhash0.1
rl_json0.15.1  Rtcl1.2.2  scrolldata2  sdx1.0  snack2.2  snap70.1
sqlite3.47.2  stardom0.42  starsync1.0  stbimage1.2  stringfileinfo0.2
 sun-valley-ttk  tangoicons0.1  tbcload1.7  tclcan0.1
tclcompiler1.7.1  tclcsv2.4.3  TclCurl7.22.0  tclepeg0.4
tclJBlend2.1.0  tcllib1.21  tcllibc1.21  TclMagick0.46  tclrmq1.4.5
tclsoap1.6.8  tcltaglib1.1  tcluvc0.1  tclws2  tclx8.6  tdbc1.1.1
tdbcjdbc0.2  tdbcmysql1.1.5  tdbcodbc1.1.1  tdbcpostgres1.1.1
tdom0.9.3  tfirmata  thread2.8.5  tile-extras  Tix8.4.3  tkbugz
tkchat1.500  tkcon2.7  tkconclient1.0  Tkhtml3.0  tkinspect5.1.6
tkled0.1  tklib0.7  TkMC1.0  tknotebook0.1  tkpath0.3.3
tksqlite0.5.13  tksvg0.14  Tktable2.11  tkvlc0.8  Tkzinc3.3.6
tls1.6.9  tnc0.3.0  tomato1  topcua0.5  touchcal0.1  treectrl2.4.2
Trf2.1.4  trofs0.4.9  tserialport1.1  ttk-themes  twdbgi0.1  udp1.0.11
 ukaz2.1  ulis  unqlite0.3.8  upnp0.2  v4l20.1  vcd0.1  vectcl0.3
VecTcLab  vectcltk0.2  vfs1.4.2  vnc0.5  vqtcl4.1  vu2.3  WavReader0.1
 wibble0.4  wscope-1.0b  www2  xdgicons1.0  yeti0.4.2
---------------------------------------------------------
I suspect that none of them use exception handling or TLS. If you show me some
example code, I'm more than happy to include that as an additional testcase.

Thanks!
         Jan Nijtmans

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[Christian W. <Chr...@t-...>]

On 01/15/2025 12:36 PM, Jan Nijtmans wrote:

> ...
> The LUCK framework supports the following (more than 150!) dll's being loaded
> by MemoryModule:
> ...
> I suspect that none of them use exception handling or TLS. If you show me some
> example code, I'm more than happy to include that as an additional testcase.

I'm not aware of any extension using TLS indeed.

There's at least one extension coded in C++ which uses exceptions internally but
proper maps them to TCL_ERROR for external call sites. This is the olden tclodbc.

Yes, I know, this proves formally exactly nothing, zilch. And since the
undroidwish/vanillawish binaries are only build using outdated mingw cross
compilers, even sub zero.

Anyway, for me the whole discussion is somewhat tedious. Let's make that into
an optional experimental feature. And, if need be, write a disclaimer as long
as the MemoryModule itself. And life can go on.

BR,
Christian

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[<apn...@ya...>]

Sigh..another long repetitive post (sorry Christian for the tedium, but
I do not enjoy this either. Really!). But this time I have
sample test cases that purportedly illustrate the problem. Folks can
skip the middle if you want and go directly to the test cases later.

Jan,

Perhaps I was not clear enough in the issues I was raising. The topic is
complex enough which is why I linked to the blogs and source code in my
previous post. Everything I know comes from there. Explicit illustrative
test cases are detailed below but first, here's why your test cases do not
address my concerns *at all*.

These are the two *simplest* scenarios that are potential TLS issues.

- Loading of two DLL's within a single thread. Your test loads one DLL
into two threads. That's not the same thing. I've added a test for this
which illustrates the data corruption.

- Loading of a DLL *after* a thread is running from a different thread.
Your test loads the extension *before* the thread is created. I have not
bothered to test this. I'm not sure how to until the two test cases
below are fixed.

The above do not even involve the more complex scenarios when the TLS
grows enough to need reallocation, the use of constructors to initialize TLS
variables and so on.

Your explanation with respect to exception handling also misses the mark.
The issue does not have to with Tcl's interfaces at all but strictly apply
to the extension's internal implementation. *If* the extension uses nested
exceptions, the loader needs to translate the table mapping address ranges
to corresponding exception handlers. That is the issue, nothing to do
with how exceptions are passed to and from Tcl. And as I stated, I do
not know if MemoryModule does this properly even with PR #91. 
I do know the update MemoryModulePP does a whole lot more in this
regard than MemoryModule and that has me wondering why. 

You assume it works until proven otherwise. I assume it doesn't work unless
proven otherwise. That's the fundamental disconnect we have.

I don't see ASLR tables and CFG tables to be handled at all. I don't if this means
something will fail or just that the benefits of those features will
be disabled.

As far as the LUCK tests go, I do not know what was tested but just loading
packages is completely meaningless. Or as Christian
himself phrased it, it means zilch. It's like saying Tcl multithreading
is race-free because packages load!

TEST CASES

But onto the main purpose of this post - the illustrative test case which is
extremely simple - just load two extensions that have implicit thread
variables. In the apn-tip-709 branch, I've added test memorymodule-3.0 to
memorymodule.test with a slight modification to your memorymoduletest
extension to build two variants. To run it, build two versions of your
dltest/memorymoduletest extension as follows:

-------
cd win
nmake /f makefile.vc OPTS=memorymodule (builds Tcl with load from memory enabled)
nmake /f makefile.vc clean
nmake /f makefile.vc (builds memorymodule extension)
nmake -f makefile.vc BUILD2=1 (build memorymodule2 extension)
zip memorymoduletest.zip *.dll pkgIndex.tcl
cd ..
Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
------

Output is 

------
---- Result was:
15 16 17
---- Result should have been (exact matching):
15 15 16
==== memorymodule-3.0 FAILED
------

illustrating that the two DLL's stomp on each other.

Of course, tests can have bugs as well so do review my changes. But
nevertheless, building Tcl with memory loading disabled,

-----
rmdir/s/q Release_AMD64_VC1939
nmake /f makefile.vc (build Tcl with load from memory disabled)
Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
-----

shows

------
memorymodule.test:      Total   5       Passed  1       Skipped 4       Failed  0
------

which is a strong indicator the test case is not the one at fault.

A second test which isolates the cause is memorymoduletest-3.1. This
retrieves the _tls_index (see http://www.nynaeve.net/?p=185) which must
be different for every DLL loaded. If we try this an interactive shell
without memory loading,

-----
% zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
//zipfs:/memorymoduletest
%  lappend auto_path //zipfs:/memorymoduletest
C:/Tcl/magic/lib/tcl9.0 C:/Tcl/magic/lib //zipfs:/memorymoduletest
% package require memorymoduletest
1.0.0
% package require memorymoduletest2
1.0.0
% memorymoduletest2::GetTlsIndex
5
%
% memorymoduletest::GetTlsIndex
4
-----

As you see the two modules have different and non-0 _tls_index values.

With memory module loading enabled however, both Dll's have the *same*
_tls_index value. 

-----
c:\src\tcltk\tip-709\tcl\win>Release_AMD64_VC1939\tcltest90.exe
% zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
//zipfs:/memorymoduletest
% lappend auto_path //zipfs:/memorymoduletest
//zipfs:/lib/tcl/tcl_library //zipfs:/lib/tcl c:/src/tcltk/tip-709/tcl/win/lib //zipfs:/memorymoduletest
% package require memorymoduletest
1.0.0
% package require memorymoduletest2
1.0.0
% memorymoduletest::GetTlsIndex
0
% memorymoduletest2::GetTlsIndex
0
-----

What's more they are both 0 as MemoryModule has not
initialized them at all! It is in fact very possible _tls_index 0 is
allocated to some other module, possibly the main application and
these DLL's are stomping on its data!

I am pretty much done with TIP 709 review unless there are
significant changes. It absolutely needs a more
filled out test suite no matter what TCT decides but I am not
going to be trying to "prove" an implementation is broken
by constructing basic tests. Given there is no test group, that
onus should be on the developer, not the reviewers, to gain
confidence in the implementation. The tests I wrote could have
and should have been constructed by anyone reading that blog.

Perhaps these failures can be ignored  with the "assurance" that
Tcl extensions do not make use of implicit threads or the other features I
mentioned. I'm sure it will all work. Most of the time. I don't support this
posture myself but I understand others might differ. Note however that use
of declspec(thread) is much more common now that (a) XP is dead and
(b) all common compilers support the equivalent.

Funnily enough, future changes by Microsoft are less of a worry because if
my test above is valid, MemoryModule anyways does not match the
implementation they currently have! I'll worry about future implementations
once MemoryModule works with the current one.

PS

For anyone interested, https://godbolt.org/z/Yxs43dc3e shows the assembler
generated for a two line program using declspec(thread).

[TCLCORE] TkChat bridge down 2 days

[Colin M. <col...@ya...>]

Hello all,

The Tcler's Chat irc/jabber bridge "ijchain" has been down for two days 
now.  Is there anyone around who can give it a kick in the requisite place?

Thanks,
Colin.

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[Harald O. <har...@el...>]

Hi Ashok,
thank you for the great analysis.

What I understood:
"TLS" means "Thread Local Storage" (and not "Transport Layer Security"). 
It means, that a static variable gets its own memory place for each 
thread the program is executed for.
This memory is handled by the system and is proper to each DLL. It must 
be extendable in size, as it is not known in advance, how many threads 
will be used. So, there must be a memory management behind, when a 
thread is created. This is probably per DLL, as each DLL knows the size 
per thread.
In assembly, the GS register holds a pointer to thread local storage.
The thread locale variable is accessed in assembly by "Offset:x"+ 
*(GS+88+8*_tls_index).
The current LoadModule implementation takes the TLS pointer from the 
application (e.g. wish or tclsh) and ignores the dll ones.
This potentially leads to memory corruption.

Thanks to dive so deeply into this subject.
I think, this is critical.

Thanks for all,
Harald

Am 16.01.2025 um 19:02 schrieb apnmbx-public--- via Tcl-Core:
> Sigh..another long repetitive post (sorry Christian for the tedium, but
> I do not enjoy this either. Really!). But this time I have
> sample test cases that purportedly illustrate the problem. Folks can
> skip the middle if you want and go directly to the test cases later.
> 
> Jan,
> 
> Perhaps I was not clear enough in the issues I was raising. The topic is
> complex enough which is why I linked to the blogs and source code in my
> previous post. Everything I know comes from there. Explicit illustrative
> test cases are detailed below but first, here's why your test cases do not
> address my concerns *at all*.
> 
> These are the two *simplest* scenarios that are potential TLS issues.
> 
> - Loading of two DLL's within a single thread. Your test loads one DLL
> into two threads. That's not the same thing. I've added a test for this
> which illustrates the data corruption.
> 
> - Loading of a DLL *after* a thread is running from a different thread.
> Your test loads the extension *before* the thread is created. I have not
> bothered to test this. I'm not sure how to until the two test cases
> below are fixed.
> 
> The above do not even involve the more complex scenarios when the TLS
> grows enough to need reallocation, the use of constructors to initialize TLS
> variables and so on.
> 
> Your explanation with respect to exception handling also misses the mark.
> The issue does not have to with Tcl's interfaces at all but strictly apply
> to the extension's internal implementation. *If* the extension uses nested
> exceptions, the loader needs to translate the table mapping address ranges
> to corresponding exception handlers. That is the issue, nothing to do
> with how exceptions are passed to and from Tcl. And as I stated, I do
> not know if MemoryModule does this properly even with PR #91.
> I do know the update MemoryModulePP does a whole lot more in this
> regard than MemoryModule and that has me wondering why.
> 
> You assume it works until proven otherwise. I assume it doesn't work unless
> proven otherwise. That's the fundamental disconnect we have.
> 
> I don't see ASLR tables and CFG tables to be handled at all. I don't if this means
> something will fail or just that the benefits of those features will
> be disabled.
> 
> As far as the LUCK tests go, I do not know what was tested but just loading
> packages is completely meaningless. Or as Christian
> himself phrased it, it means zilch. It's like saying Tcl multithreading
> is race-free because packages load!
> 
> TEST CASES
> 
> But onto the main purpose of this post - the illustrative test case which is
> extremely simple - just load two extensions that have implicit thread
> variables. In the apn-tip-709 branch, I've added test memorymodule-3.0 to
> memorymodule.test with a slight modification to your memorymoduletest
> extension to build two variants. To run it, build two versions of your
> dltest/memorymoduletest extension as follows:
> 
> -------
> cd win
> nmake /f makefile.vc OPTS=memorymodule (builds Tcl with load from memory enabled)
> nmake /f makefile.vc clean
> nmake /f makefile.vc (builds memorymodule extension)
> nmake -f makefile.vc BUILD2=1 (build memorymodule2 extension)
> zip memorymoduletest.zip *.dll pkgIndex.tcl
> cd ..
> Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
> ------
> 
> Output is
> 
> ------
> ---- Result was:
> 15 16 17
> ---- Result should have been (exact matching):
> 15 15 16
> ==== memorymodule-3.0 FAILED
> ------
> 
> illustrating that the two DLL's stomp on each other.
> 
> Of course, tests can have bugs as well so do review my changes. But
> nevertheless, building Tcl with memory loading disabled,
> 
> -----
> rmdir/s/q Release_AMD64_VC1939
> nmake /f makefile.vc (build Tcl with load from memory disabled)
> Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
> -----
> 
> shows
> 
> ------
> memorymodule.test:      Total   5       Passed  1       Skipped 4       Failed  0
> ------
> 
> which is a strong indicator the test case is not the one at fault.
> 
> A second test which isolates the cause is memorymoduletest-3.1. This
> retrieves the _tls_index (see http://www.nynaeve.net/?p=185) which must
> be different for every DLL loaded. If we try this an interactive shell
> without memory loading,
> 
> -----
> % zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
> //zipfs:/memorymoduletest
> %  lappend auto_path //zipfs:/memorymoduletest
> C:/Tcl/magic/lib/tcl9.0 C:/Tcl/magic/lib //zipfs:/memorymoduletest
> % package require memorymoduletest
> 1.0.0
> % package require memorymoduletest2
> 1.0.0
> % memorymoduletest2::GetTlsIndex
> 5
> %
> % memorymoduletest::GetTlsIndex
> 4
> -----
> 
> As you see the two modules have different and non-0 _tls_index values.
> 
> With memory module loading enabled however, both Dll's have the *same*
> _tls_index value.
> 
> -----
> c:\src\tcltk\tip-709\tcl\win>Release_AMD64_VC1939\tcltest90.exe
> % zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
> //zipfs:/memorymoduletest
> % lappend auto_path //zipfs:/memorymoduletest
> //zipfs:/lib/tcl/tcl_library //zipfs:/lib/tcl c:/src/tcltk/tip-709/tcl/win/lib //zipfs:/memorymoduletest
> % package require memorymoduletest
> 1.0.0
> % package require memorymoduletest2
> 1.0.0
> % memorymoduletest::GetTlsIndex
> 0
> % memorymoduletest2::GetTlsIndex
> 0
> -----
> 
> What's more they are both 0 as MemoryModule has not
> initialized them at all! It is in fact very possible _tls_index 0 is
> allocated to some other module, possibly the main application and
> these DLL's are stomping on its data!
> 
> I am pretty much done with TIP 709 review unless there are
> significant changes. It absolutely needs a more
> filled out test suite no matter what TCT decides but I am not
> going to be trying to "prove" an implementation is broken
> by constructing basic tests. Given there is no test group, that
> onus should be on the developer, not the reviewers, to gain
> confidence in the implementation. The tests I wrote could have
> and should have been constructed by anyone reading that blog.
> 
> Perhaps these failures can be ignored  with the "assurance" that
> Tcl extensions do not make use of implicit threads or the other features I
> mentioned. I'm sure it will all work. Most of the time. I don't support this
> posture myself but I understand others might differ. Note however that use
> of declspec(thread) is much more common now that (a) XP is dead and
> (b) all common compilers support the equivalent.
> 
> Funnily enough, future changes by Microsoft are less of a worry because if
> my test above is valid, MemoryModule anyways does not match the
> implementation they currently have! I'll worry about future implementations
> once MemoryModule works with the current one.
> 
> PS
> 
> For anyone interested, https://godbolt.org/z/Yxs43dc3e shows the assembler
> generated for a two line program using declspec(thread).

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[<apn...@ya...>]

Better summary than mine.

Only the last point - _tcl_index = 0 corresponding to the application - is pure speculation on my part. It is tangential to the main argument anyways.

/Ashok

-----Original Message-----
From: Harald Oehlmann <har...@el...> 
Sent: Friday, January 17, 2025 6:23 PM
To: tcl...@li...
Subject: Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

Hi Ashok,
thank you for the great analysis.

What I understood:
"TLS" means "Thread Local Storage" (and not "Transport Layer Security"). 
It means, that a static variable gets its own memory place for each 
thread the program is executed for.
This memory is handled by the system and is proper to each DLL. It must 
be extendable in size, as it is not known in advance, how many threads 
will be used. So, there must be a memory management behind, when a 
thread is created. This is probably per DLL, as each DLL knows the size 
per thread.
In assembly, the GS register holds a pointer to thread local storage.
The thread locale variable is accessed in assembly by "Offset:x"+ 
*(GS+88+8*_tls_index).
The current LoadModule implementation takes the TLS pointer from the 
application (e.g. wish or tclsh) and ignores the dll ones.
This potentially leads to memory corruption.

Thanks to dive so deeply into this subject.
I think, this is critical.

Thanks for all,
Harald

Am 16.01.2025 um 19:02 schrieb apnmbx-public--- via Tcl-Core:
> Sigh..another long repetitive post (sorry Christian for the tedium, but
> I do not enjoy this either. Really!). But this time I have
> sample test cases that purportedly illustrate the problem. Folks can
> skip the middle if you want and go directly to the test cases later.
> 
> Jan,
> 
> Perhaps I was not clear enough in the issues I was raising. The topic is
> complex enough which is why I linked to the blogs and source code in my
> previous post. Everything I know comes from there. Explicit illustrative
> test cases are detailed below but first, here's why your test cases do not
> address my concerns *at all*.
> 
> These are the two *simplest* scenarios that are potential TLS issues.
> 
> - Loading of two DLL's within a single thread. Your test loads one DLL
> into two threads. That's not the same thing. I've added a test for this
> which illustrates the data corruption.
> 
> - Loading of a DLL *after* a thread is running from a different thread.
> Your test loads the extension *before* the thread is created. I have not
> bothered to test this. I'm not sure how to until the two test cases
> below are fixed.
> 
> The above do not even involve the more complex scenarios when the TLS
> grows enough to need reallocation, the use of constructors to initialize TLS
> variables and so on.
> 
> Your explanation with respect to exception handling also misses the mark.
> The issue does not have to with Tcl's interfaces at all but strictly apply
> to the extension's internal implementation. *If* the extension uses nested
> exceptions, the loader needs to translate the table mapping address ranges
> to corresponding exception handlers. That is the issue, nothing to do
> with how exceptions are passed to and from Tcl. And as I stated, I do
> not know if MemoryModule does this properly even with PR #91.
> I do know the update MemoryModulePP does a whole lot more in this
> regard than MemoryModule and that has me wondering why.
> 
> You assume it works until proven otherwise. I assume it doesn't work unless
> proven otherwise. That's the fundamental disconnect we have.
> 
> I don't see ASLR tables and CFG tables to be handled at all. I don't if this means
> something will fail or just that the benefits of those features will
> be disabled.
> 
> As far as the LUCK tests go, I do not know what was tested but just loading
> packages is completely meaningless. Or as Christian
> himself phrased it, it means zilch. It's like saying Tcl multithreading
> is race-free because packages load!
> 
> TEST CASES
> 
> But onto the main purpose of this post - the illustrative test case which is
> extremely simple - just load two extensions that have implicit thread
> variables. In the apn-tip-709 branch, I've added test memorymodule-3.0 to
> memorymodule.test with a slight modification to your memorymoduletest
> extension to build two variants. To run it, build two versions of your
> dltest/memorymoduletest extension as follows:
> 
> -------
> cd win
> nmake /f makefile.vc OPTS=memorymodule (builds Tcl with load from memory enabled)
> nmake /f makefile.vc clean
> nmake /f makefile.vc (builds memorymodule extension)
> nmake -f makefile.vc BUILD2=1 (build memorymodule2 extension)
> zip memorymoduletest.zip *.dll pkgIndex.tcl
> cd ..
> Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
> ------
> 
> Output is
> 
> ------
> ---- Result was:
> 15 16 17
> ---- Result should have been (exact matching):
> 15 15 16
> ==== memorymodule-3.0 FAILED
> ------
> 
> illustrating that the two DLL's stomp on each other.
> 
> Of course, tests can have bugs as well so do review my changes. But
> nevertheless, building Tcl with memory loading disabled,
> 
> -----
> rmdir/s/q Release_AMD64_VC1939
> nmake /f makefile.vc (build Tcl with load from memory disabled)
> Release_AMD64_VC1939\tclsh90.exe ..\tests\memorymodule.test -constraints memorymoduletest -match *-3.0
> -----
> 
> shows
> 
> ------
> memorymodule.test:      Total   5       Passed  1       Skipped 4       Failed  0
> ------
> 
> which is a strong indicator the test case is not the one at fault.
> 
> A second test which isolates the cause is memorymoduletest-3.1. This
> retrieves the _tls_index (see http://www.nynaeve.net/?p=185) which must
> be different for every DLL loaded. If we try this an interactive shell
> without memory loading,
> 
> -----
> % zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
> //zipfs:/memorymoduletest
> %  lappend auto_path //zipfs:/memorymoduletest
> C:/Tcl/magic/lib/tcl9.0 C:/Tcl/magic/lib //zipfs:/memorymoduletest
> % package require memorymoduletest
> 1.0.0
> % package require memorymoduletest2
> 1.0.0
> % memorymoduletest2::GetTlsIndex
> 5
> %
> % memorymoduletest::GetTlsIndex
> 4
> -----
> 
> As you see the two modules have different and non-0 _tls_index values.
> 
> With memory module loading enabled however, both Dll's have the *same*
> _tls_index value.
> 
> -----
> c:\src\tcltk\tip-709\tcl\win>Release_AMD64_VC1939\tcltest90.exe
> % zipfs mount dltest/memorymoduletest.zip [file join [zipfs root] memorymoduletest]
> //zipfs:/memorymoduletest
> % lappend auto_path //zipfs:/memorymoduletest
> //zipfs:/lib/tcl/tcl_library //zipfs:/lib/tcl c:/src/tcltk/tip-709/tcl/win/lib //zipfs:/memorymoduletest
> % package require memorymoduletest
> 1.0.0
> % package require memorymoduletest2
> 1.0.0
> % memorymoduletest::GetTlsIndex
> 0
> % memorymoduletest2::GetTlsIndex
> 0
> -----
> 
> What's more they are both 0 as MemoryModule has not
> initialized them at all! It is in fact very possible _tls_index 0 is
> allocated to some other module, possibly the main application and
> these DLL's are stomping on its data!
> 
> I am pretty much done with TIP 709 review unless there are
> significant changes. It absolutely needs a more
> filled out test suite no matter what TCT decides but I am not
> going to be trying to "prove" an implementation is broken
> by constructing basic tests. Given there is no test group, that
> onus should be on the developer, not the reviewers, to gain
> confidence in the implementation. The tests I wrote could have
> and should have been constructed by anyone reading that blog.
> 
> Perhaps these failures can be ignored  with the "assurance" that
> Tcl extensions do not make use of implicit threads or the other features I
> mentioned. I'm sure it will all work. Most of the time. I don't support this
> posture myself but I understand others might differ. Note however that use
> of declspec(thread) is much more common now that (a) XP is dead and
> (b) all common compilers support the equivalent.
> 
> Funnily enough, future changes by Microsoft are less of a worry because if
> my test above is valid, MemoryModule anyways does not match the
> implementation they currently have! I'll worry about future implementations
> once MemoryModule works with the current one.
> 
> PS
> 
> For anyone interested, https://godbolt.org/z/Yxs43dc3e shows the assembler
> generated for a two line program using declspec(thread).

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[Jan N. <jan...@gm...>]

Op vr 17 jan 2025 om 13:53 schreef Harald Oehlmann
<har...@el...>:
> What I understood:
> ...
> This is probably per DLL, as each DLL knows the size
> per thread.
>...
> This potentially leads to memory corruption.

... probably ... potentially ......   That's a lot of speculation!!!!!!

Please don't do that!  The real explanation is much simpler,
I will come back on that.

Regards,
          Jan Nijtmans

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[<apn...@ya...>]

To Jan's point on the branch having to be closed due to the MPL *if* the TIP
is not approved, I do think the proposal for a license should be TIP'ed
separately from the functionality of a TIP. They are really orthogonal
issues and an "MPL permitted in all future code" TIP should be approved on
separately for exactly this reason. If the MPL TIP passes through, there is
no reason for the MemoryModule file (and future MPL-licensed contributions)
not to be committed in the source repository. The vote for the "load from
memory" feature TIP should be separate and control whether it becomes part
of Tcl implementation. That's only my opinion.

 

With respect to Jan's comment that I'm exaggerating the risk, I freely admit
:-) I'm prone to that when it comes to undocumented behaviors so my comments
are colored accordingly. For that reason, I researched further a summary of
which I will post separately for folks to decide for themselves.

 

And regarding "badly written", let me clarify I never meant to imply
MemoryModule is badly written. The code may be very well written but could
still be incomplete in terms of required features.

 

/Ashok

 

-----Original Message-----
From: Jan Nijtmans <jan...@gm...> 
Sent: Saturday, January 11, 2025 1:19 AM
To: Kevin Walzer <kw...@co...>
Cc: tcl...@li...
Subject: Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

 

Op vr 10 jan 2025 om 19:58 schreef Kevin Walzer:

> I'd caution against relying on undocumented/private API's unless there is
a GREAT reason to do so. Windows has been remarkably stable for Tcl/Tk, and
it would be shame for API churn (typical of Apple) to make its way into that
source tree.

 

That's part of the point here. I think, being able to load dll's in memory
is

such a GREAT reason. Ashok is IMHO exaggerating on how bad

MemoryModule is, looking at the code it is - actually - programmed

quite well. Well enough that it is picked up by other projects, and

other people were contributing back their fixes. I wrote testcases

for various situations Ashok pointed out to be problematic: All of

those worked fine! A "great" reason would be enough for me,

it doesn't even need to be GREAT.

 

Since I was not able to convince Ashok, it's now on our plate to

decide.

 

Part of the question is also non-technical. Should the quality

of code be a reason to accept or decline it in one of our

repositories? If TIP #705 "Affirm Tcl License" wouldn't be

there, TIP #709 wouldn't have been written at all. It also

means that - if TIP #709 is declined, I have to close the

branch and cannot even work on it any more, trying to

make it even more GREAT than it already is ;-)

 

Hope this helps,

    Jan Nijtmans

 

 

_______________________________________________

Tcl-Core mailing list

 <mailto:Tcl...@li...> Tcl...@li...

 <https://lists.sourceforge.net/lists/listinfo/tcl-core>
https://lists.sourceforge.net/lists/listinfo/tcl-core

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[Harald O. <har...@el...>]

Am 12.01.2025 um 10:36 schrieb apnmbx-public--- via Tcl-Core:
> To Jan's point on the branch having to be closed due to the MPL *if* the 
> TIP is not approved, I do think the proposal for a license should be 
> TIP'ed separately from the functionality of a TIP. They are really 
> orthogonal issues and an "MPL permitted in all future code" TIP should 
> be approved on separately for exactly this reason. If the MPL TIP passes 
> through, there is no reason for the MemoryModule file (and future MPL- 
> licensed contributions) not to be committed in the source repository. 
> The vote for the "load from memory" feature TIP should be separate and 
> control whether it becomes part of Tcl implementation. That's only my 
> opinion.
> 
> With respect to Jan's comment that I'm exaggerating the risk, I freely 
> admit :-) I'm prone to that when it comes to undocumented behaviors so 
> my comments are colored accordingly. For that reason, I researched 
> further a summary of which I will post separately for folks to decide 
> for themselves.
> 
> And regarding "badly written", let me clarify I never meant to imply 
> MemoryModule is badly written. The code may be very well written but 
> could still be incomplete in terms of required features.
> 
> /Ashok
+1 on all points.

It is not ok to bundle the TIPs.

I would love to have the memory module feature in TCL 9.1.
It would allow to have an all-in-one executable which works without 
worries in the normal case.

Starkits had the same feature to leave temporary dlls behind. Due to 
that, secondary DLL's are avoided and, for example, tdom has all 
secondary dll's statically linked.
Ashok pointed out, that temporary dll's may be banned due to virus 
checkers etc. I often supposed and supported this argument, but all 
cases I saw had other reasons.
Specially the magic "Internet Download flag" for a dll often causes 
issues. This only happens, if I supply the dll beside the executable, 
not as temporare dll. And those issues arise by windows update out of 
the sudden.

Also Marcs proposal to put them always in the same file is good. But 
with the memory module, we don't need that solution neither.
Jan already started a checksum check, if it is the right dll and to 
reuse it.

Thanks for all,
Harald

Re: [TCLCORE] CFV warning: TIP #709: MPL Licence for MemoryModule

[<apn...@ya...>]

Following up on my previous post, here is the basis and some justification
for my concerns. (I'm no expert so consider it a Google search dump)

 

My conclusions ...

 

1. Afaict, MemoryModule does not fully implement the features required for
emulating Windows loader behaviour even today.

 

2. Even if it did, the internals could change in any update, a service pack
even. The possibility of this is probably small, but not predictable. So I'm
not going to comment further on this. We can all make our own estimates of
this risk.

 

With respect to (1) however, I would ask TIP 709 reviewers to at least skim
the following resources. It does not really require Windows knowledge, just
basic data structures and a general idea about loaders.

 

*	The series of blog articles on the Windows loader, ending with the
most important one at http://www.nynaeve.net/?p=189 which essentially
reverse engineers the loader's role in TLS (thread local storage, not the
protocol :-)).

 

*	The sample code for the above at
http://www.nynaeve.net/Code/VistaImplicitTls.cpp

 

*	An implementation MemoryModulePP, derived from MemoryModule, at
https://github.com/bb107/MemoryModulePP

 

My conclusion going through the links above is that MemoryModule is still
primarily based on the much simpler and less capable pre-Vista Windows
loader. In particular, it allocates memory, maps sections (including
resource sections), sets up VM flags, maps symbol addresses, registers
exception handlers and TLS callbacks.

 

Per my reading of just the TLS-related portions,

 

*	It does not allocate slots for the *implicit* TLS entries and
initialize them in the loading thread.

 

*	It does not update the TLS map for existing threads by reallocating
(if necessary) the TLS storage and updating the in-use slot map.

 

For the complexity associated with the seemingly simple tasks above, compare
with the implementations in the aforementioned source code links to see how
much work is involved and missing from MemoryModule.

 

>From the above, I conclude at least the TLS-related functionality in
MemoryModule is incomplete. If we were to continue with TIP 709.

 

1.	One could assume the occurrences of the above TLS in the *Tcl* world
are minimal and just ignore the issue and hope for the best. Based on
Christian W.'s tests with LUCK, there is a good chance this might work. I am
however a believer in Murphy's law. And it does not address other potential
non-TLS issues listed later.

 

2.	One could document the restrictions (not sure exactly what those
would be) imposed on memory loading and leave it to the application writer
to check and verify. This may not be easy for them because of the use of
third party libraries in an extension and even calls made to Win32 API's.
Personally I am generally not in favour of features that work some of the
time under some (ill-defined) conditions on some platforms. Note that it is
*not* possible, at least as currently implemented, to make the determination
at load time and use the fallback method of copying to disk. Failures to
initialize TLS structures show up as corrupted memory at some later point.

 

3.	Switch the TIP 709 implementation from MemoryModule to the updated
MemoryModulePP which seems to be more modern. The concerns here would be
whether (a) even that is complete, (b) introduction of C++ into the Tcl code
base and (c) its reliance on yet more additional intrusive third party
libraries that hook system API's.

 

A word about testing - Jan mentioned test cases he wrote that pass. Assuming
the test dll is memorymoduletest.c, the tests do not address my concerns.
The use of a single tls variable, with a single thread, an integer scalar
with value 0, and no constructor will not suffice for the purpose. There are
is no test for nested exception handlers. A broader set of tests is needed.
IIUC, Christian W. has done extensive testing with LUCK and found no issues.
I am not sure which extensions were used and the manner (number of threads,
size and *type* of the TLS allocations etc.) but perhaps those could be used
as a starting point. Note however that failures are not easily detected
(akin to race conditions).

 

The above only looked at TLS but there are other potential issues

 

*	Exception handler registration. MemoryModule uses the simple
RtlAddFunctionTable call. MemoryModulePP on the other hand has a much more
complex implementation. I am not sure of the reason, and it may be
inconsequential but the fact that MemoryModulePP is derived from
MemoryModule and still found it necessary to change that simple call makes
me wonder.

 

*	Newer Windows loader features like CFG (control flow graphs) and
ASLR do not seem to be handled at all. Perhaps this may only matter if
builds make use of those features, but I would think that is the direction
Tcl should be moving in anyways for security reasons.

 

Another interesting tidbit Google threw up. Python's py2exe make use of
MemoryModule, more or less the same code base as TIP 709. This looked
promising in support of 709; however, note it is not part of Python but a
separate application and is architected very differently. More disturbing is
https://github.com/py2exe/py2exe/pull/214#issuecomment-2476280385. Quoting
(bolded emphasis mine),

 

In windows due to how LoadLibraryA/W loads DLLs with the TLS information
that initialization of the python core when bundle_files  <= 1 is not
possible. The resulting files must be the exe with the python core dll as a
separate file from the exe in order for it to be initialized properly. That
is unless the dll gets packed into the win32 resource section and just prior
to it calling any python C api functions for it to write the dll to the
disk. But even then that can break the built in C extensions when the exe
does not directly import the core dll in it's IMPORT_ADDRESS_TABLE.

 

I don't quite grok that but does not give me a comfortable feeling that the
dll needs to be a separate file (not a single file app then!) or written to
disk. I could not follow all the references and links in those tickets to
figure out what the current situation is (the above link was Nov 2024)

 

Jan also mentioned many applications that were already using MemoryModule
and there are a lot of forks but outside of py2exe, I could not locate
anything substantial outside of malware and malware analysers :-)

 

I hope folks, Jan and Christian W. in particular as they have the most
interest and knowledge in this space, will review and weigh the above
factors prior to voting. Even better, if some can alleviate my concerns.

 

In my own view, more study is needed before bringing 709 to a vote. As it
stands *now*, I would not be in favour though I am all in for the feature.
Contrary to what it may seem from the above, I was googling to convince
myself my fears were unfounded.

 

/Ashok