#235 On-screen keyboard can crash Tux Paint

[William Kendrick]

Report received via Twitter (h/t @Miyagi_Andel):

A very inconsistent crash seems to happen when the onscreen keyboard is on and text or label is selected. I use Windows 10 so I checked the event viewer. Faulting module name: libiconv-2.dll. Exception code: 0xc0000005(Access Violation).

Empty stdout.txt, but stderr.txt exists, though it doesn't appear to contain anything related to this crash. (Fontconfig warnings, libpng warnings, and a couple Pango-WARNINGs.)

[Pere Pujal i Carabantes]

Reproduced in an emulated W10.
Found also a strange behavior in Linux:
Click repeatedly the purple arrows to change the keyboard layout, time to time the letters go fade, nearly blank, the symbols stays black.
Not sure if it is the same problem, as it doesn't crash in Linux.
The fact that it doesn't happen every time I change the layout makes me think on a race condition...

[William Kendrick]

I cannot reproduce the behavior you noticed in Linux, @perepujal (with Tux Paint Git Master). Clicking the left/right purple arrows in the OSK does not seem to behave badly for me. :(

[William Kendrick]

There is some Win32-specific code in src/tuxpaint.c that uses iconv functions while saving, in do_png_embed_data(). However, I haven't heard that this bug is related to saving. How did you happen to reproduce it, @perepujal?

We also have an mtw() function declared in src/tuxpaint.c, and utilized while loading an image, in load_info_about_label_surface(). The part of the code that calls it is also Win32-specific (therefore, that whole function could be wrapped in an #ifdef WIN32 test, frankly).

I'm guessing the culprit is somewhere in src/onscreen_keyboard.c itself, which also uses iconv functions. And, again, they are only being declared and used under a Win32 environment.

A mtw() function is declared here as well, described as a "workaround using iconv to get a functionallity somewhat approximate as mbstowcs()". Then "mbstowcs" is #define'd to call this mtw function.

They are NOT identical, however. In tuxpaint.c we do ui16 = malloc(255);, whereas in the OSK code, we do ui16 = malloc(sizeof(Uint16) * 255); (which is, of course, twice as big).

We should clean this up.

We should have one function, in its own C source and H header file; perhaps "win32_mtw.c" & ".h"?

It should do the 'right thing'. I'm not sure where the 255 size for mallocs even comes from, but it seems dangerous.

Extraneous code should be removed. e.g., what is wch for? It's malloc()'d, but never used, and worse yet, never free()'d!

Cc @dolphin6k

[Pere Pujal i Carabantes]

To make the Windows version crash I just need to (re)generate the onscreen keyboard.
If the first click on text or label tools doesn't crashes Tux Paint, I just keep clicking the purple arrows that will change the keyboard.

Oh, the problem I see in Linux is with the sdl2.0 branch, I checked master and indeed it works fine.

[Shin-ichi TOYAMA]

I'm not quite sure if it is correct, but just disabling windows specific code in onscreen_keyboard.c seems to solve this bug.
I guess many windows specific code had been tweaked to supple APIs lacking in mingw/msys at that time. But some of them seems to have been already implimented in current MinGW/MSYS2.

[Shin-ichi TOYAMA]

I think disabling such old mingw/msys specific code might affect build for Windows XP/2000.
However, I think it is the time to give up supporting such old system considering the plan to move to SDL2.

[William Kendrick]

Thanks, Shin-ichi. Can you see if you can also remove our "mtw()" function from src/tuxpaint.c as well, and drop the Win32-specific #ifdef/#else/#endif that was using it, inside load_info_about_label_surface()? If not, let me know, and I can look into it (though I cannot test on Windows at this time!)

Looking at downloads-per-week numbers (at https://sourceforge.net/projects/tuxpaint/files/tuxpaint/0.9.26/), right now I see
* 83.54% of the various Win32 downloads are for the x86_64 installer & ZIP versions
* 12.45% are for the i686 installer & ZIP versions, and
* 4.01% are for the 2K/XP installer & ZIP versions

Looking at Google Analytics data for all of September 2021, only about 425 hits came from users running Windows XP (the majority) or Windows 2000 (literally only 6 users). They account for 0.24% of site traffic.

(Based on that, I kind of wonder how many users are downloading a sub-optimal version. i.e., Windows Vista, 7, 8, and 10 users accidentally downloading the XP/2000 version of Tux Paint!)

These days, the majority of downloads and site traffic comes from India, and when I further break down the Analytics data by OS, then country, and filtering on "XP" and sorting by number of users, I see these top ones:
* 133 (33%) India
* 62 (14%) Greece
* 42 (16%) Ukraine
* 16 (3%) Poland

And for Windows 2000, the 6 users were in the Ukraine (3), the US (2), and India (1).

In my opinion, I agree we can drop support for XP/2000. (We'll of course continue providing downloads -- version 0.9.26 & friends -- for those OSes on the http://tuxpaint.org/download/older/ page).

To clarify, this means we will almost certainly drop XP/2000 as of this upcoming version: 0.9.27 (which I was planning to also be our final SDL1.2-based version of Tux Paint).

[Shin-ichi TOYAMA]

Removed windows specific mtw() stuff from tuxpaint.c and it compiled with no error.
I commited it to the git repo and built windows binaries for 0.9.26 again, and put them on my web server as 0.9.26-2.
See https://z1.plala.jp/tuxpaint/release/0.9.26-2/

[William Kendrick]

Ok, going to assume things are mended, and closing this. Thanks!