[Maria-developers] Review: bb-10.8-wlad-MDEV-26713
Hi Wlad, You've done a great piece of work. Thanks and congrats. I generally like all the changes. Have a few comments only, please see below. commit 1411d5f40ceaf2eecabbcf68588743439003f4a2 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Fri Nov 19 11:42:46 2021 +0100 Windows : incorrect handling of non-ASCIIs in get_tty_password Why no MDEV? + wchar_t wbuf[80]; + char to[80*3]; /* there is at most 3 bytes per wchar, in any codepage */ + wchar_t *pos=wbuf,*end=wbuf + array_elements(wbuf)-1; Isn't it "at most 4 bytes per char"? ############################################################# commit 7af691ca11c95a385aff4889a56870d70843e78d Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Fri Nov 19 12:03:48 2021 +0100 MDEV-27090: Windows client - ReadConsoleA does not work correctly with UTF8 codepage Corresponding Windows bug https://github.com/microsoft/terminal/issues/4551 Use ReadConsoleW instead and convert to console's input codepage, to workaround. Also, disable VT sequences in the console output, as we do not knows what type of data comes with SELECT, we do not want VT escapes there. Remove my_cgets() + while (nchars > 0) + { + if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') + break; + wstrbuf[--nchars]= 0; + } + + wstrbuf[nchars]= 0; The inner assignment wstrbuf[--nchars]=0 inside every iteration is not needed. I'd write it this way: for ( ; nchars > 0 ; nchars--) { if (wstrbuf[nchars - 1] != '\n' && wstrbuf[nchars - 1] != '\r') break; } wstrbuf[nchars]= 0; But the outer assignment to 0 does not seem to be needed either. Why do it? my_convert() does not need the trailing zero. It uses only sizes passed. So this should be enough: + strbuf[len]= 0; - pthread_auto_mutex_decl(my_conio_cs); - - /* lock the console for the current process*/ - if (pthread_auto_mutex_lock(my_conio_cs, GetCurrentProcessId(), INFINITE)) - { There was a mutex lock inside my_cgets(). Why was it needed before the patch and why is it not needed any more? ##################################################### commit 5c09e9ce4f922fe9674f3f85329742c0c5498a28 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Fri Nov 19 14:03:51 2021 +0100 MDEV-27092 Windows - services that have non-ASCII characters do not work with activeCodePage=UTF8 CreateServiceA, OpenServiceA, and couple of other functions do not work correctly with non-ASCII character, in the special case where application has defined activeCodePage=UTF8. ... - Copyright (c) 2011, 2012, Monty Program Ab + Copyright (c) 2011, 2021 Monty Program Ab MariaDB Corporation? This code: + AWSTRDUP(w_ServiceName,lpServiceName); + AWSTRDUP(w_DisplayName,lpDisplayName); + AWSTRDUP(w_BinaryPathName, lpBinaryPathName); + AWSTRDUP(w_LoadOrderGroup, lpLoadOrderGroup); + AWSTRDUP(w_Dependencies, lpDependencies); + AWSTRDUP(w_ServiceStartName, lpServiceStartName); + AWSTRDUP(w_Password, lpPassword); + + free(w_ServiceName); + free(w_DisplayName); + free(w_BinaryPathName); + free(w_LoadOrderGroup); + free(w_Dependencies); + free(w_ServiceStartName); + free(w_Password); duplicates two times. Consider wrapping these variables into a class and reuse the class in both cases. ########################################################## commit d4056a28ec5ae0c9a0c3144a8e90357dc574723f Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Fri Nov 19 14:14:38 2021 +0100 MDEV-27093 Do not pass root password in clear text from mariadb-install-db.exe to bootstrap Pass the hash instead, so that bootstrap would not misinterpret it for string with different charset. The password was passed using hex encoding. But the comment says it was clear text. Wrong comment? I did not understand the change. Reading the MDEV did not make it clearer. What was wrong? How does the patch fix the problem? Can we discuss on slack? ######################################################## commit fa29a64de918d2045df3cce1d4a546de5597c0bd Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Mon Nov 22 12:29:15 2021 +0100 MDEV-26713 set activeCodePage=UTF8 for windows programs - Use corresponding entry in the manifest, as described in https://docs.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-co... - Set console codepage(s) to UTF8, if ANSI codepage is UTF8 This effectively causes UTF8 to be used as default client encoding, for Windows 1903 and later - Allow UTF8 on the command line for MTR, for the affected Windows versions Looks OK for me. ######################################################## commit b88d54132369b938ba041c0d3f97a5a212210b37 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Mon Nov 22 12:34:10 2021 +0100 MDEV-26713 : Treat codepage 65001 as utf8mb4, not just utf8 Also, fix the "UTF8" option in MSI, which is responsible for character-set-server setting Looks OK for me. ######################################################## commit b9c38470ed6e04185f7f84a056034b7254336d51 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Mon Nov 22 13:22:05 2021 +0100 MDEV-26713 utf8 support on Windows, mysql_upgrade_service part Handle upgrade - on Windows that is capable of UTF8 codepage, convert entries in my.ini from ANSI to UTF8, during upgrade Reason is that paths must now be utf8. For the client , user name, database etc should be in UTF8, too, as this is now the default charset. Should not we add an UTF8 BOM to the beginning of the file, so text editors do not break it later. The patch looks too large for me. It seems it does more than the comments above tell. Looks like convenience refactoring, but it's hard to follow. It would be easier to understand changes if you split it into two parts: 1. the refactoring that adds functions like upgrade_config_file(), fix_section(), is_mariadb_section(), fix_and_check_datadir(), I guess this refactoring is a non-functional change that should not change the behaviour, right? 2. the actual conversion part on top of it ######################################################## commit 0700393b570c6b9a7b4347d3381a7cf05d1499ba Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Tue Nov 23 12:28:28 2021 +0100 mysqltest, Windows - support background execution in 'exec' command If last character in command is ampersand, do not use my_popen/my_pclose in "exec", instead do CreateProcess() without a window. I suggest you add an MDEV for this change and explain in there why we did not need this feature earlier and why we need it now and how it relates to MDEV-26713. ######################################################## commit 7d50d83b3bcab5a3be761f98722e33ed0b01ab23 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Tue Nov 23 13:05:25 2021 +0100 MDEV-26713 utf8 support on Windows , add mysql_install_db tests Add mysql_install_db test with some i18n, for data dir and root password Looks OK for me. ######################################################## commit b2cc09fe76f526bd4cd76131a52263b13b3828a5 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Wed Nov 24 10:15:11 2021 +0100 MDEV-26713 Add test for mysql_install_db creating service, with i18 Looks OK for me. ######################################################## commit 51ce069376fec45d340563a4f7630f8a155afd64 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Fri Nov 26 18:41:35 2021 +0100 MDEV-26713 Introduce --password-codepage client setting I'm not sure we need this new options. Looks like over-engineering a bit. Is the scenario it covers really deserves documenting, learning, etc? I'd better disallow non-ASCII passwords in the future, and right now just add a warning: You're using a non-ASCII password, which is not portable. If you have problems connecting to the database, please ask the administrator to change the password. ######################################################## commit b155c4036cfb3ffdaa7c272048ac9d32e3182d78 Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Mon Nov 29 19:47:36 2021 +0100 MDEV-26713 set console codepage to what user set in --default-character-set I prefer to keep the comment that you removed: - if (!fs_cset_cache) - { - char buf[10]= "cp"; - GetLocaleInfo(LOCALE_SYSTEM_DEFAULT, LOCALE_IDEFAULTANSICODEPAGE, - buf+2, sizeof(buf)-3); - /* - We cannot call get_charset_by_name here - because fs_character_set() is executed before - LOCK_THD_charset mutex initialization, which - is used inside get_charset_by_name. - As we're now interested in cp932 only, - let's just detect it using strcmp(). - */ - fs_cset_cache= - #ifdef HAVE_CHARSET_cp932 - !strcmp(buf, "cp932") ? &my_charset_cp932_japanese_ci : - #endif - &my_charset_bin; This function uses both default_charset and charset_info->cs_name.str: +static void adjust_console_codepage() +{ +#ifdef _WIN32 + const char *name= charset_info->cs_name.str; + if (my_set_console_cp(default_charset) < 0) And it seems the former is set to the latter just before the function call: default_charset= (char *)charset_info->cs_name.str; <------ HERE put_info("Charset changed", INFO_INFO); adjust_console_codepage(); I find this hard to follow. Can you please add a parameter to adjust_console_codepage() and pass either charset_info->cs_name.str (or default_charset) when calling? Also, I think this function should only be called when default_charset was some explicit name, not MYSQL_AUTODETECT_CHARSET_NAME. I think the logic should be: 1. If --default-character-set=auto, then detect it from the console. 2. If --default-character-set=csname, then set the console CP to csname. Now in case of N1 it seems to detect the character set from the console, then sets the console to what has been detected. Is the last step really needed in case of auto-detection? ######################################################## commit b38c341cc392630c798eb754f87338d37ec95cdf Author: Vladislav Vaintroub <wlad@mariadb.com> Date: Sun Dec 5 17:08:18 2021 +0100 Fix race condition in test. Do not change console codepage in a possibly shared console. Start new console for chcp. Can you extend the comment and tell what actually happened before this change? Greetings.
Hi Wlad, On 12/13/21 2:15 AM, Vladislav Vaintroub wrote:
Hi Bar,
Thanks for review, much appreciated! I addressed most of the comments in the new push https://github.com/MariaDB/server/compare/bb-10.8-wlad-MDEV-26713 <https://github.com/MariaDB/server/compare/bb-10.8-wlad-MDEV-26713>
commit 1411d5f40ceaf2eecabbcf68588743439003f4a2
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Fri Nov 19 11:42:46 2021 +0100
+ wchar_t wbuf[80];
+ char to[80*3]; /* there is at most 3 bytes per wchar, in any codepage */
+ wchar_t *pos=wbuf,*end=wbuf + array_elements(wbuf)-1;
Isn't it "at most 4 bytes per char"?
You’re right. I missed GB18030, single UTF16 wchar_t can expand to four GB18030 bytes . But for UTF8, single UTF16 wchar_t can’t expand to 4 bytes, if you mean that?
- One wchar_t on Windows can encode U+0000..U+FFFF (BMP) only, so it produces not more than 3 bytes in UTF8 indeed. - A 4-byte UTF8 (SMP) can be produced from two wchar_t's only (a high+low surrogate pair). - All multi-byte character sets (except UTF-32 and GB18030) can produce 3 bytes per character at most. - GB18030 can produce 4-byte sequences from BMP characters, i.e. from a single wchar_t. What will happen with this code if one starts a client program with GB18030 as a console code page? The code that we're discussing here uses GetConsoleCP(), so I guess it CAN be GB18030, even though we don't support it yet. - UTF-32 produces 4 byte characters. What will happen if one starts a client program with UTF-32 as a console codepage? Also, please consider adding a comment. It's not obvious where 3 comes from, especially for a Linux addicted reader where a single wchar_t encode the entire Unicode range.
#############################################################
commit 7af691ca11c95a385aff4889a56870d70843e78d
Author: Vladislav Vaintroub <wlad@mariadb.com >
- pthread_auto_mutex_decl(my_conio_cs);
...
There was a mutex lock inside my_cgets().
Why was it needed before the patch and why is it not needed any more?
The old code did more than it had to. We do not need mutex locking for reading line from console window, and we do not use threads in command line client.
Right. The old code seemed to be redundant. Strange, why this lock was there.
########################################################
commit b9c38470ed6e04185f7f84a056034b7254336d51
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Mon Nov 22 13:22:05 2021 +0100
......
Should not we add an UTF8 BOM to the beginning of the file,
so text editors do not break it later.
It might render it unusable for our own stuff, see MySQL Bugs: #67880: mysql 5.6.9 on windows does not start if my.ini file is in utf8 format <https://bugs.mysql.com/bug.php?id=67880>
I see no reason for having BOM now. Windows’ own editors, Notepad by defaults stores in BOM-less UTF-8 (on systems we target with this work at last), VS Code, Notepad++ . Some Linux tools however would have problems with BOM, in particular “cat” . Anyway, its been a long while since I’ve seen UTF8 being not correctly detected and handled by any editor.
Okey, let's go without BOM. Our code is not always BOM friendly indeed.
########################################################
commit 51ce069376fec45d340563a4f7630f8a155afd64
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Fri Nov 26 18:41:35 2021 +0100
MDEV-26713 Introduce --password-codepage client setting
I'm not sure we need this new options.
Looks like over-engineering a bit.
Ok,I agree new parameter is not nice. And, found a way to handle that without , using --default-character-set MDEV-26713 allow users with non-UTF8 passwords to login after upgrade. · MariaDB/server@b45c887 (github.com) <https://github.com/MariaDB/server/commit/b45c8872742221b921a02118fcd33281925dbf83>
I’m firmly convinced, that if we are about to break connectivity, and we know about it, we must to offer something else than “contact your administrator”. That administrator could be the guy who implemented password validation plugin to force at last one non-ASCII character (it is safer, say some people on the internet, harder to break for hackers), and now that poor admin is busy fixing a hundreds of “reset my password” requests. Who knows, I do not think this scenario is entirely unrealistic.
We have no clue how much impact breaking non-ASCII passwords would be. We never warned users, that such passwords would not work, or are “not portable” .It is impossible to run an automated check, whether such passwords were used, prior to upgrade, as we never stored the actual password.
At the end, users are not guilty in the mess we created ourselves by handling passwords as bytes, rather than strings. Thus user should not be punished for something he had not broke. Instead, an explanation, and an easy workaround that allows to connect after upgrade, must be in release notes.
I agree and I like that you found a way to do it without a new option.
#####################################################
commit 5c09e9ce4f922fe9674f3f85329742c0c5498a28
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Fri Nov 19 14:03:51 2021 +0100
MDEV-27092 Windows - services that have non-ASCII characters do not
>work with activeCodePage=UTF8
This code:
+ AWSTRDUP(w_ServiceName,lpServiceName);
....
duplicates two times. Consider wrapping these variables into a class
and reuse the class in both cases.
It duplicates because those functions are a trivial wrappers for 2 C functions, with similar, but not exact the same signature I considered wrapping, a C++ class, spent some time trying. It turned out looking too artificial, and added more code actually that it removed, which made me believe there is no good abstraction for this case. I’m leaving it as-is.
Okey.
########################################################
commit b155c4036cfb3ffdaa7c272048ac9d32e3182d78
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Mon Nov 29 19:47:36 2021 +0100
MDEV-26713 set console codepage to what user set in
>--default-character-set
...
+static void adjust_console_codepage()
...<skip>...
Also, I think this function should only be called when
default_charset was some explicit name, not MYSQL_AUTODETECT_CHARSET_NAME.
I think the logic should be:
1. If --default-character-set=auto, then detect it from the console.
2. If --default-character-set=csname, then set the console CP to csname.
Now in case of N1 it seems to detect the character set from the console,
then sets the console to what has been detected.
Is the last step really needed in case of auto-detection?
It seems like it would not be needed. However our autodetection is loose, and this has some implications :
Let's say the console codepage is cp437. And we got some old Windows, pre Win10 1903 .
cp437 would be autodetected as MySQL cp850. There are 46 codepoint point differences between cp437 and cp850, (s. https://en.wikipedia.org/wiki/Code_page_850, differences marked yellow), so those codepages are not really very similar, differing in ~20% of the code points.
In the logic I used, console would be is fixed from 437 to 850, thus showing French accented characters for French accented characters in data, rather than random Greek.
So, I’d think last step was fine.
Thanks. This explains. In case of a non-exact detection it's good idea to adjust the console. Can you please add a comment to the code?
########################################################
commit b38c341cc392630c798eb754f87338d37ec95cdf
Author: Vladislav Vaintroub <wlad@mariadb.com >
Date: Sun Dec 5 17:08:18 2021 +0100
Fix race condition in test.
Can you extend the comment and tell what actually happened
before this change?
I rebased into into some other commit, but so I'll describe here what happened here.
On the “new” buildbot, Windows host runs mysql-test-run.pl --parallel=64 . It is an “old” version of Windows in terms of this work, Windows 10 build 1803, so it uses legacy codepage detection via console CP. And it runs a test client_charset_win.test, which is
chcp 1257 > NULL & $MYSQL --default-character-set=auto -e "select @@character_set_client"
The machine is US English Windows, with default console codepage is 437 . However test would sometimes, actually most of the times, show cp850 instead of expected cp1257
What happens here, there is that of builbot slave runs perl (MTR), and all MTR subprocesses share the same console object, the one of python process, which is builbot slave.
Now, none of the subprocesses processes would be using console, and there is no visible window even. Output is redirected for all processes, as well as input, where appropriate. Yes, one of the clients managed to change the shared console codepage to 850, in the tiny time window between chcp and the client startup . I’d guess this was fixing of loosely autodetected cp850 on cp437, as I described earlier.
So the fix is avoid changing console codepage, unless there is an actual console output.
Thank for the explanation. I still have some questions. Let's talk on slack.
Best regards,
Wlad
*From: *Alexander Barkov <mailto:bar@mariadb.com> *Sent: *Friday, 10 December 2021 14:40 *To: *maria-developers <mailto:maria-developers@lists.launchpad.net>; Vladislav Vaintroub <mailto:wlad@mariadb.com> *Subject: *Review: bb-10.8-wlad-MDEV-26713
Hi Wlad,
You've done a great piece of work. Thanks and congrats.
..
participants (2)
-
Alexander Barkov -
Vladislav Vaintroub