From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 68662385E007 for ; Fri, 16 Jul 2021 20:28:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 68662385E007 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-525-6WWM3L97M-q7MqlsQTSdrw-1; Fri, 16 Jul 2021 16:28:41 -0400 X-MC-Unique: 6WWM3L97M-q7MqlsQTSdrw-1 Received: by mail-qk1-f200.google.com with SMTP id u14-20020a05620a120eb02903b8bd5c7d95so1475625qkj.12 for ; Fri, 16 Jul 2021 13:28:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=0Gor7Bkvh++dntWXU9bJ1yX9VidexYkF0OsgI701tvo=; b=eN0UYR10vISKIXr+mCcxhe2fZkueiRT6NzGF8E0ywxxvmyy7hv2GrdkMdhZeygxzks wxHrxSJPdKZiwtMFuOkwIlM+7P7RX/8UcEGsynvT44FTQ+Q1m52xAbOyi6/icJ6lbYLu 4Bc8/A+9856WEtUYtYPK9WTLN24+VT+tRhNj3iheyKsQsmY0tm08q3lu2A2Deu5tVzPR Vv2NOr6TzdpLmauHKW+yq5utGFyq/MP9kzQvAci67e0TpCUKdSEzJ7iOkRmbROoYmL5R jycrVN7g9nk4vuzYarThfnIVmDTKOdMWScUyaQSLMMksSRd9IPr3Wm5wzFHhrJWnu/ZO EEgA== X-Gm-Message-State: AOAM531n752QI7IWhsmnzYd9XekMEQ9W4s1LBzqZfQmEJpAh3eU8Epn3 yCjwNfEpvV+mCJWsalFTO4SU1idZwTFEHpBaQtGBjXP1QgwEgbsQi96TTZVPA/3RQGnIdaGgoyC psuEIpnToArTPe919NoIw X-Received: by 2002:a05:6214:252b:: with SMTP id gg11mr12041014qvb.15.1626467320657; Fri, 16 Jul 2021 13:28:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxPrahctn20OXl9hryRiM36spqOBkBdVakaGweI3MZKqz2gxC4Ljc4Q9V9xIVr/WZcBvuvM2Q== X-Received: by 2002:a05:6214:252b:: with SMTP id gg11mr12040996qvb.15.1626467320314; Fri, 16 Jul 2021 13:28:40 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id w6sm1682886qtt.36.2021.07.16.13.28.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Jul 2021 13:28:39 -0700 (PDT) Subject: Re: [PATCH] setlocale: Fail if iconv module for charset is not present [BZ #27996] To: Siddhesh Poyarekar , libc-alpha@sourceware.org Cc: fweimer@redhat.com References: <20210630085642.2661589-1-siddhesh@sourceware.org> From: Carlos O'Donell Organization: Red Hat Message-ID: <15d97714-1294-5373-770f-b285f985a7b1@redhat.com> Date: Fri, 16 Jul 2021 16:28:38 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210630085642.2661589-1-siddhesh@sourceware.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Jul 2021 20:28:45 -0000 On 6/30/21 4:56 AM, Siddhesh Poyarekar via Libc-alpha wrote: > setlocale currently succeeds even if the requested locale uses a > charset that does not have a converter module installed. Check for > existence of the charset (either the one requested through the input > name or the one needed by the selected locale file) and fail if it > doesn't. I think this is an important part of being able to reliably reduce the number of installed iconv converters as a method for hardening an install. I think we need stricter testing in __gconv_module_exists before this is complete. Given that you are testing both directions the name of the functions probably needs changing. You are no longer testing for a module but for the modules required to transit the charset encoding into and out of glibc. > For testing, test6.c has been recycled because it was unused. The > test verifes that loading test5 and test6 locales fail because both > locales have charsets without a converter, viz. test5 and test6 > respectively. Please indicate that test6.c has been *removed* not recycled (commit message update). > --- > iconv/gconv_db.c | 25 ++++++ > iconv/gconv_int.h | 2 + > locale/findlocale.c | 62 +++++++++----- > localedata/Makefile | 8 +- > localedata/tests/test6.c | 137 ------------------------------- > localedata/tst-invalid-charset.c | 31 +++++++ > 6 files changed, 103 insertions(+), 162 deletions(-) > delete mode 100644 localedata/tests/test6.c > create mode 100644 localedata/tst-invalid-charset.c > > diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c > index af868e8023..547a6d0a44 100644 > --- a/iconv/gconv_db.c > +++ b/iconv/gconv_db.c > @@ -698,6 +698,31 @@ do_lookup_alias (const char *name) > return found != NULL ? (*found)->toname : NULL; > } > > +bool > +__gconv_module_exists (const char *name) > +{ > + /* Ensure that the configuration data is read. */ > + __gconv_load_conf (); OK. Load modules available on the system. > + > + name = do_lookup_alias (name) ?: name; OK. > + > + struct gconv_module *root = __gconv_modules_db; > + > + while (root != NULL) > + { > + int cmpres; > + > + cmpres = strcmp (name, root->from_string); I think this is incomplete. (a) We must have a converter that goes from name->INTERNAL. (b) We must have a converter that goes from INTERNAL->name. You are only checking for (a) here, which means we can read input of the charset, but we can't write it out via printf and I don't think that would match user expectations. > + if (cmpres == 0) > + return true; OK. I guess we don't care to match the ->same entries. > + else if (cmpres < 0) > + root = root->left; > + else > + root = root->right; OK. > + } > + > + return false; > +} OK. > > int > __gconv_compare_alias (const char *name1, const char *name2) > diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h > index 30a9286be2..6a52275700 100644 > --- a/iconv/gconv_int.h > +++ b/iconv/gconv_int.h > @@ -269,6 +269,8 @@ libc_hidden_proto (__gconv_transliterate) > extern int __gconv_compare_alias (const char *name1, const char *name2) > attribute_hidden; > > +/* Return true if NAME exists either as a module or an alias. */ > +extern bool __gconv_module_exists (const char *name) attribute_hidden; OK. New function. > > /* Builtin transformations. */ > #ifdef _LIBC > diff --git a/locale/findlocale.c b/locale/findlocale.c > index ab09122b0c..10dfe2aef3 100644 > --- a/locale/findlocale.c > +++ b/locale/findlocale.c > @@ -98,6 +98,15 @@ valid_locale_name (const char *name) > return 1; > } > > +static bool > +codeset_has_module (const char *codeset) > +{ > + char *ccodeset = (char *) alloca (strlen (codeset) + 3); OK. NULL + up to 2 chars from strip. > + strip (ccodeset, codeset); OK. Use strip to normalize. > + > + return __gconv_module_exists (ccodeset); > +} > + > struct __locale_data * > _nl_find_locale (const char *locale_path, size_t locale_path_len, > int category, const char **name) > @@ -200,6 +209,10 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > /* Memory allocate problem. */ > return NULL; > > + /* The requested codeset does not have a converter, don't use it. */ > + if (codeset != NULL && !codeset_has_module (codeset)) > + return NULL; OK. > + > /* If exactly this locale was already asked for we have an entry with > the complete name. */ > locale_file = _nl_make_l10nflist (&_nl_locale_file_list[category], > @@ -248,6 +261,33 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > return NULL; > } > > + /* Get the codeset information from the locale file. */ > + static const int codeset_idx[] = > + { > + [__LC_CTYPE] = _NL_ITEM_INDEX (CODESET), > + [__LC_NUMERIC] = _NL_ITEM_INDEX (_NL_NUMERIC_CODESET), > + [__LC_TIME] = _NL_ITEM_INDEX (_NL_TIME_CODESET), > + [__LC_COLLATE] = _NL_ITEM_INDEX (_NL_COLLATE_CODESET), > + [__LC_MONETARY] = _NL_ITEM_INDEX (_NL_MONETARY_CODESET), > + [__LC_MESSAGES] = _NL_ITEM_INDEX (_NL_MESSAGES_CODESET), > + [__LC_PAPER] = _NL_ITEM_INDEX (_NL_PAPER_CODESET), > + [__LC_NAME] = _NL_ITEM_INDEX (_NL_NAME_CODESET), > + [__LC_ADDRESS] = _NL_ITEM_INDEX (_NL_ADDRESS_CODESET), > + [__LC_TELEPHONE] = _NL_ITEM_INDEX (_NL_TELEPHONE_CODESET), > + [__LC_MEASUREMENT] = _NL_ITEM_INDEX (_NL_MEASUREMENT_CODESET), > + [__LC_IDENTIFICATION] = _NL_ITEM_INDEX (_NL_IDENTIFICATION_CODESET) > + }; > + const struct __locale_data *data; > + const char *locale_codeset; > + > + data = (const struct __locale_data *) locale_file->data; > + locale_codeset = (const char *) data->values[codeset_idx[category]].string; > + assert (locale_codeset != NULL); > + > + /* The locale codeset does not have a converter, don't use it. */ > + if (locale_codeset[0] != '\0' && !codeset_has_module (locale_codeset)) > + return NULL; OK. Move this code earlier and look for a converter for the codeset. > + > /* The LC_CTYPE category allows to check whether a locale is really > usable. If the locale name contains a charset name and the > charset name used in the locale (present in the LC_CTYPE data) is > @@ -256,31 +296,9 @@ _nl_find_locale (const char *locale_path, size_t locale_path_len, > in the locale name. */ > if (codeset != NULL) > { > - /* Get the codeset information from the locale file. */ > - static const int codeset_idx[] = > - { > - [__LC_CTYPE] = _NL_ITEM_INDEX (CODESET), > - [__LC_NUMERIC] = _NL_ITEM_INDEX (_NL_NUMERIC_CODESET), > - [__LC_TIME] = _NL_ITEM_INDEX (_NL_TIME_CODESET), > - [__LC_COLLATE] = _NL_ITEM_INDEX (_NL_COLLATE_CODESET), > - [__LC_MONETARY] = _NL_ITEM_INDEX (_NL_MONETARY_CODESET), > - [__LC_MESSAGES] = _NL_ITEM_INDEX (_NL_MESSAGES_CODESET), > - [__LC_PAPER] = _NL_ITEM_INDEX (_NL_PAPER_CODESET), > - [__LC_NAME] = _NL_ITEM_INDEX (_NL_NAME_CODESET), > - [__LC_ADDRESS] = _NL_ITEM_INDEX (_NL_ADDRESS_CODESET), > - [__LC_TELEPHONE] = _NL_ITEM_INDEX (_NL_TELEPHONE_CODESET), > - [__LC_MEASUREMENT] = _NL_ITEM_INDEX (_NL_MEASUREMENT_CODESET), > - [__LC_IDENTIFICATION] = _NL_ITEM_INDEX (_NL_IDENTIFICATION_CODESET) > - }; > - const struct __locale_data *data; > - const char *locale_codeset; > char *clocale_codeset; > char *ccodeset; > > - data = (const struct __locale_data *) locale_file->data; > - locale_codeset = > - (const char *) data->values[codeset_idx[category]].string; > - assert (locale_codeset != NULL); > /* Note the length of the allocated memory: +3 for up to two slashes > and the NUL byte. */ > clocale_codeset = (char *) alloca (strlen (locale_codeset) + 3); > diff --git a/localedata/Makefile b/localedata/Makefile > index 14e04cd3c5..797523b250 100644 > --- a/localedata/Makefile > +++ b/localedata/Makefile > @@ -128,7 +128,7 @@ ld-test-names := test1 test2 test3 test4 test5 test6 test7 > ld-test-srcs := $(addprefix tests/,$(addsuffix .cm,$(ld-test-names)) \ > $(addsuffix .def,$(ld-test-names)) \ > $(addsuffix .ds,test5 test6) \ > - test6.c trans.def) > + trans.def) OK. Remove unused test6.c > > fmon-tests = n01y12 n02n40 n10y31 n11y41 n12y11 n20n32 n30y20 n41n00 \ > y01y10 y02n22 y22n42 y30y21 y32n31 y40y00 y42n21 > @@ -158,7 +158,7 @@ tests = $(locale_test_suite) tst-digits tst-setlocale bug-iconv-trans \ > tst-leaks tst-mbswcs1 tst-mbswcs2 tst-mbswcs3 tst-mbswcs4 tst-mbswcs5 \ > tst-mbswcs6 tst-xlocale1 tst-xlocale2 bug-usesetlocale \ > tst-strfmon1 tst-sscanf bug-setlocale1 tst-setlocale2 tst-setlocale3 \ > - tst-wctype tst-iconv-math-trans > + tst-wctype tst-iconv-math-trans tst-invalid-charset OK. > tests-static = bug-setlocale1-static > tests += $(tests-static) > ifeq (yes,$(build-shared)) > @@ -402,6 +402,7 @@ $(objpfx)tst-langinfo-setlocale-static.out: tst-langinfo.sh \ > $(evaluate-test) > > $(objpfx)tst-digits.out: $(objpfx)tst-locale.out > +$(objpfx)tst-invalid-charset.out: $(objpfx)tst-locale.out This needs a comment saying why it depends on tst-locale.out e.g. because it wants to use the compiled test locales which have no charset converters. > $(objpfx)tst-mbswcs6.out: $(addprefix $(objpfx),$(CTYPE_FILES)) > endif > > @@ -461,7 +462,8 @@ $(objpfx)mtrace-tst-leaks.out: $(objpfx)tst-leaks.out > $(common-objpfx)malloc/mtrace $(objpfx)tst-leaks.mtrace > $@; \ > $(evaluate-test) > > -bug-setlocale1-ENV-only = LOCPATH=$(objpfx) LC_CTYPE=de_DE.UTF-8 > +bug-setlocale1-ENV-only = GCONV_PATH=$(common-objpfx)iconvdata \ > + LOCPATH=$(objpfx) LC_CTYPE=de_DE.UTF-8 OK. Fix test. > bug-setlocale1-static-ENV-only = $(bug-setlocale1-ENV-only) > > $(objdir)/iconvdata/gconv-modules: > diff --git a/localedata/tests/test6.c b/localedata/tests/test6.c > deleted file mode 100644 > index edb5fe4a5f..0000000000 > --- a/localedata/tests/test6.c > +++ /dev/null > @@ -1,137 +0,0 @@ > -/* Test program for character classes and mappings. > - Copyright (C) 1999-2021 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - Contributed by Ulrich Drepper , 1999. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library; if not, see > - . */ > - > -#include > -#include > -#include > - > - > -int > -main (void) > -{ > - const char lower[] = "abcdefghijklmnopqrstuvwxyz"; > - const char upper[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > -#define LEN (sizeof (upper) - 1) > - const wchar_t wlower[] = L"abcdefghijklmnopqrstuvwxyz"; > - const wchar_t wupper[] = L"ABCDEFGHIJKLMNOPQRSTUVWXYZ"; > - int i; > - int result = 0; > - > - setlocale (LC_ALL, "test6"); > - > - for (i = 0; i < LEN; ++i) > - { > - /* Test basic table handling (basic == not more than 256 characters). > - The charmaps swaps the normal lower-upper case meaning of the > - ASCII characters used in the source code while the Unicode mapping > - in the repertoire map has the normal correspondents. This test > - shows the independence of the tables for `char' and `wchar_t' > - characters. */ > - > - if (islower (lower[i])) > - { > - printf ("islower ('%c') false\n", lower[i]); > - result = 1; > - } > - if (! isupper (lower[i])) > - { > - printf ("isupper ('%c') false\n", lower[i]); > - result = 1; > - } > - > - if (! islower (upper[i])) > - { > - printf ("islower ('%c') false\n", upper[i]); > - result = 1; > - } > - if (isupper (upper[i])) > - { > - printf ("isupper ('%c') false\n", upper[i]); > - result = 1; > - } > - > - if (toupper (lower[i]) != lower[i]) > - { > - printf ("toupper ('%c') false\n", lower[i]); > - result = 1; > - } > - if (tolower (lower[i]) != upper[i]) > - { > - printf ("tolower ('%c') false\n", lower[i]); > - result = 1; > - } > - > - if (tolower (upper[i]) != upper[i]) > - { > - printf ("tolower ('%c') false\n", upper[i]); > - result = 1; > - } > - if (toupper (upper[i]) != lower[i]) > - { > - printf ("toupper ('%c') false\n", upper[i]); > - result = 1; > - } > - > - if (iswlower (wupper[i])) > - { > - printf ("iswlower (L'%c') false\n", upper[i]); > - result = 1; > - } > - if (! iswupper (wupper[i])) > - { > - printf ("iswupper (L'%c') false\n", upper[i]); > - result = 1; > - } > - > - if (iswupper (wlower[i])) > - { > - printf ("iswupper (L'%c') false\n", lower[i]); > - result = 1; > - } > - if (! iswlower (wlower[i])) > - { > - printf ("iswlower (L'%c') false\n", lower[i]); > - result = 1; > - } > - > - if (towupper (wlower[i]) != wupper[i]) > - { > - printf ("towupper ('%c') false\n", lower[i]); > - result = 1; > - } > - if (towlower (wlower[i]) != wlower[i]) > - { > - printf ("towlower ('%c') false\n", lower[i]); > - result = 1; > - } > - > - if (towlower (wupper[i]) != wlower[i]) > - { > - printf ("towlower ('%c') false\n", upper[i]); > - result = 1; > - } > - if (towupper (wupper[i]) != wupper[i]) > - { > - printf ("towupper ('%c') false\n", upper[i]); > - result = 1; > - } > - } > - > - return result; > -} > diff --git a/localedata/tst-invalid-charset.c b/localedata/tst-invalid-charset.c > new file mode 100644 > index 0000000000..46a5198c66 > --- /dev/null > +++ b/localedata/tst-invalid-charset.c > @@ -0,0 +1,31 @@ > +/* Test program to verify that setlocale fails for charsets that do not have a > + converter. > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > + > + > +int > +main (void) > +{ > + /* Fail if setlocale succeeds for any of these locales. */ Please add a comment in localedata/Makefile where ld-test-names is set and mention that this test depends on test5 and test6 having no charset converters. > + return (setlocale (LC_ALL, "test5") != NULL > + || setlocale (LC_ALL, "test6") != NULL); > +} > -- Cheers, Carlos.