Change more Cursor -> Tk_Cursor to fix exceptions on Strawberry Perl 64-bit

[shaiku]

On Windows Strawberry 64-bit, sizeof(Cursor) is 4 and sizeof(Tk_Cursor) is 8.

Tk_FreeOptions() casts ptr to Tk_Cursor* and dereferences it to compare with None. If the widget uses Cursor instead of Tk_Cursor then the derefernce will include 4 bytes of adjacent data may erroneously call Tk_FreeCursor with an invalid cursor argument type, leading to panic("Tk_FreeCursor received unknown cursor argument");

This patch fixes some long-standing bugs when destroying HList and Tree objects:
https://rt.cpan.org/Public/Bug/Display.html?id=67889
https://groups.google.com/forum/#!topic/comp.lang.perl.tk/chS7mupl5Hw
http://www.perl-community.de/bat/poard/thread/16244

[chrstphrchvz]

Is this is the ideal approach for this? Even though this is what was done in the past (cf. ee70e48), I'm inclined to think there are still other potentially broken things sharing the same cause as this.

If Cursor is supposed to be a pointer type, then shouldn't it be defined in terms of a type large enough to hold one in the first place?

Right now Cursor is defined by

typedef XID Cursor;

in X.h, which in turn is defined by

typedef unsigned long XID;

which as of 08de0f2 is defined by

#if defined(_WIN64) && defined(_MSC_VER)
typedef __int64 XID;
#else
typedef unsigned long XID;
#endif

specifically in order to work with Strawberry Perl gcc on Windows 64 bit (RT #60707)…does this approach no longer work?

There's also the disadvantage that this diverges from upstream Tix source code (which could very well be wrong), even though it hasn't been updated in a while—maybe not since 2008.

[shaiku]

I guess a question for upstream tcl/tk is why is there both a Cursor and a Tk_Cursor, especially when a pointer to a Cursor is later being cast to a Tk_Cursor pointer. There should be only one type and a survey of the code shows that Tk_Cursor is far more frequent.

Your point is good, however, that the issue of Cursor being sized wrong on 64-bit Strawberry builds should be addressed. The problem appears to have been introduced by this change: 08de0f2

Instead of checking for _MSC_VER, the simple inclusion of stdint.h should resolve the issue of __int64 not being defined under gcc.

Perhaps there was an assumption that the 64-bit gcc would define a long as 64-bits but at least the version included with 64-bit Strawberry does not, so it is better to use int64_t or __int64.

[chrstphrchvz]

(I misunderstood the original issue of dereferencing pointers to different sized types, and somehow confused it for Cursor and Tk_Cursor being directly treated as pointers.)

So does reverting 08de0f2 fix the issue? Would this reintroduce issues for older Strawberry Perl users (is that something to be concerned about)?

I would guess Tk_Cursor is a helpful abstraction Tcl/Tk uses, in contrast to Cursor which would be directly tied to X11. Maybe Tix's authors were just accustomed to using Cursor instead of Tk_Cursor and got away with it, and the right thing to do indeed might be to change to Tk_Cursor.

Regarding stdint.h: does Perl/Tk already compile using C99 or later? (I know Perl itself can't use stdint.h, since that's not part of C89, but I don't know if Perl modules should relax that restriction.)

[shaiku]

Reverting 08de0f2 doesn't fix the issue -- it results in a compiler error since __int64 is undefined. I think in the Microsoft compiler they are defined in the language extensions without requiring stdint.h.

Perl/Tk compiles fine with C99 as far as I know.

[chrstphrchvz]

Regarding stdint.h: does Perl/Tk already compile using C99 or later? (I know Perl itself can't use stdint.h, since that's not part of C89, but I don't know if Perl modules should relax that restriction.)

Perl/Tk compiles fine with C99 as far as I know.

What I mean is, can Perl/Tk require C99 (if it does not already)? (It would be nice if it could, so that things like stdint.h/inttypes.h can be used.)

[eserte]

Perl/Tk has some kind of configuration system (see myConfig and the config subdirectory) --- so if the question is the existence of a C header file, then it could be checked here. E.g. something like

$define{'HAVE_INTTYPES_H'} = 1     if ($Config{'i_inttypes'});

and then include inttypes.h conditionally if HAVE_INTTYPES_H is defined.

[cboleary]

Hi all,
This change is pretty important to an application I am distributing to customers since it crashes at random when they are using one of my HLIST based guis with large data sets.

Is there any plan to merge this?

If not, does it seem like a good Idea for me to clone this branch and make my own version of Tk to use with my application?
(I'm a little out of my league to try something like this, but I'm willing to give it a go)

[cboleary]

When looking at the changes you made
I noticed a few other places where the plain Cursor type is still being used...
I highlighted the ones you changed,
I am am wondering if any others need to change?

Unchanged
./pTk/mTk/generic/tkPointer.c: Cursor cursor;
./pTk/mTk/generic/tkPointer.c: Cursor cursor = None;
./pTk/mTk/generic/tkPointer.c: Cursor cursor;

Changed in this Pull Request
./pTk/mTk/tixGeneric/tixHList.h: Cursor cursor; /* Current cursor for window, or None. /
./pTk/mTk/tixGeneric/tixInputO.c: Cursor cursor; / Current cursor for window, or None. /
./pTk/mTk/tixGeneric/tixNBFrame.c: Cursor cursor; / Current cursor for window, or None. /
Unchanged
./pTk/mTk/unix/tkUnixCursor.c: Cursor cursor = None;
./pTk/mTk/unix/tkUnixCursor.c: Cursor cursor;
./pTk/mTk/win/stubs.c: Cursor cursor;
./pTk/mTk/win/tkWinCursor.c: TkpCursor cursor;
./pTk/mTk/xlib/X11/Xlib.h: Cursor cursor; / cursor to be displayed (or None) */

There are also a few TkpCursors thrown in for good measure

[shaiku]

cboleary, It has been a couple years since I looked at this but I believe the other usages of Cursor that you reference don't cause bugs either because they are only used for assignments and not pointed to as the wrong type or the member in the struct is defined as Cursor and not Tk_Cursor.

typedef struct {
[... removed for brevity ...]
Cursor cursor; /* cursor to be displayed (or None) */
} XSetWindowAttributes;

The bug applies only in the case when the cursor is destroyed by Tk_FreeOptions() (when it's a member of the object itself) and I believe my patch covers those cases. We've been using my patched version at work for the last two years and have never seen this exception return.

[cboleary]

@shaiku Thanks for the quick reply!
@eserte has made quite a few changes following your pull request, so I want to build his latest release with your changes....

Once I do, I'll need to update the installed version of Tk on my strawberry perl installations...
I've never rebuilt an officially distributed module and then tried to use it as the latest version,
Any suggestions on how to accomplish this seamlessly?

[shaiku]

I include it when I build my strawberry distribution. You can download the official TK tar.gz off of metacspan and patch it manually, then save back into the tar.gz archive.
I use perldis_strawberry (https://metacpan.org/pod/distribution/Perl-Dist-Strawberry/script/perldist_strawberry) to build my distribution. You will pass perldist_strawberry a .pp config file on the command line. In that config file add a line to the modules section like

### NEXT STEP ###########################
{
    plugin => 'Perl::Dist::Strawberry::Step::InstallModules',
    modules => [
        # Add Tk with Tk_Cursor / HList bugfig
        'file://C:/Temp/build_strawberry/Tk-804.034.tar.gz',

that points to the updated tk archive in the strawberry build directory. Set up all the other modules and add any additional third party modules. After building you get a strawberry installer customized exactly for your needs.

[cboleary]

hanks again for the quick info! If I follow, you are using that module to build a strawberry perl installer which includes all the modules you want upfront using that .pp file?T if so that's pretty cool, I had no idea that was an option.... I hate learning this stuff late in the game... makes me take too many left turns when I should be focused on other things :) So if you want the latest version of Tk you would pull it and patch that one (which is what I'm about to do) I'm a bit surprised you didn't change the Version tag a little bit to ensure you knew its your modified Tk, but I guess you already know that since they are using your own custom installer.

…

-Best Bill O'Leary Founder CadEnhance EDA Productivity Tools (214) 280-8804 *-Your EDA Tools.... Automated* www.CadEnhance.com <http://www.cadenhance.com/>

On Tue, Nov 10, 2020 at 3:16 PM shaiku ***@***.***> wrote: I include it when I build my strawberry distribution. You can download the official TK tar.gz off of metacspan and patch it manually, then save back into the tar.gz archive. I use perldis_strawberry ( https://metacpan.org/pod/distribution/Perl-Dist-Strawberry/script/perldist_strawberry) to build my distribution. You will pass perldist_strawberry a .pp config file on the command line. In that config file add a line to the modules section like ### NEXT STEP ########################### { plugin => 'Perl::Dist::Strawberry::Step::InstallModules', modules => [ # Add Tk with Tk_Cursor / HList bugfig 'file://C:/Temp/build_strawberry/Tk-804.034.tar.gz', that points to the updated tk archive in the strawberry build directory. Set up all the other modules and add any additional third party modules. After building you get a strawberry installer customized exactly for your needs. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#40 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEASWOAWJ3DKE7MPWXRDNLSPGUSHANCNFSM4GAZXSQQ> .

[shaiku]

That's right, look at this for a full example https://github.com/StrawberryPerl/Perl-Dist-Strawberry/blob/master/share/64bit-5.32.0.1.pp

I didn't bother changing the version string. Actually disappointed that my changes weren't accepted by now -- they cause no harm as far as I've been able to determine and fix a legitimate bug / crash. Maybe this will get some renewed attention after your feedback.

[cboleary]

@shaiku do you have a Ko-fi (https://ko-fi.com/account)?
I'd like to reward you for the valuable insight and fix you provided me.

@eserte... same goes for you... my companies tools depend heavily on the all the great work you've done on perl-tk