Reset converter state after second wchar_t output (Bug 25734)

  Andreas,

Does this look good to you?

I admit it is odd that the converter returns 0-bytes consumed on the
second conversion, it could be enhanced to correctly show that we're
really consume 1-byte at a time, but that's another bug.

8< --- 8< --- 8< ---
>From ea8a94fafeebc87ccd030c825484328a6da43f47 Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Fri, 27 Mar 2020 17:03:36 -0400
Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734)

An input BIG5-HKSCS character may be converted into at most 2 whcar_t
characters. After outputting the second whcar_t character (which was
saved in the converter state) we must reset the state. If we fail
to reset the state we will be stuck continually copying that
character to the output even if we have further input to consider.

We add a new test case that covers the 4 BIG5-HKSCS characters
that may become 2 wchar_t characters.
---
 iconvdata/Makefile                        |   6 +-
 iconvdata/big5hkscs.c                     |   3 +
 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 146 ++++++++++++++++++++++
 3 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
  

On 3/27/20 5:18 PM, Carlos O'Donell wrote:
> Andreas,
> 
> Does this look good to you?
> 
> I admit it is odd that the converter returns 0-bytes consumed on the
> second conversion, it could be enhanced to correctly show that we're
> really consume 1-byte at a time, but that's another bug.
> 

Tom,

Thanks for your review!

If you want you can give a "Reviewed-by:"

Andreas,

I'd like to see a review from you given your experience with BIG5.

v2
- Fixed typo.
- Added mbsinit test (thanks Tom).
- Fixed logical flaw in test loop in do_test.

> 8< --- 8< --- 8< ---

>From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001
From: Carlos O'Donell <carlos@redhat.com>
Date: Fri, 27 Mar 2020 17:03:36 -0400
Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734)

An input BIG5-HKSCS character may be converted into at most 2 whcar_t
characters. After outputting the second whcar_t character (which was
saved in the converter state) we must reset the state. If we fail
to reset the state we will be stuck continually copying that
character to the output even if we have further input to consider.

We add a new test case that covers the 4 BIG5-HKSCS characters
that may become 2 wchar_t characters.
---
 iconvdata/Makefile                        |   6 +-
 iconvdata/big5hkscs.c                     |   3 +
 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++
 3 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c

diff --git a/iconvdata/Makefile b/iconvdata/Makefile
index c83962f351..71ddef09db 100644
--- a/iconvdata/Makefile
+++ b/iconvdata/Makefile
@@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules))
 ifeq (yes,$(build-shared))
 tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \
 	tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \
-	bug-iconv10 bug-iconv11 bug-iconv12
+	bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4
 ifeq ($(have-thread-library),yes)
 tests += bug-iconv3
 endif
@@ -340,3 +340,7 @@ tst-tables-clean:
 
 $(objpfx)gconv-modules: gconv-modules
 	cat $(sysdeps-gconv-modules) $^ > $@
+
+# Test requires BIG5HKSCS.
+$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \
+			  $(addprefix $(objpfx),BIG5HKSCS.so)
diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c
index 01fcfeba76..ef325119b1 100644
--- a/iconvdata/big5hkscs.c
+++ b/iconvdata/big5hkscs.c
@@ -17895,6 +17895,9 @@ static struct
 	else								      \
 	  ++inptr;							      \
       }									      \
+    else								      \
+      /* Clear the queue and proceed to output the saved character.  */	      \
+      *statep = 0;							      \
 									      \
     put32 (outptr, ch);							      \
     outptr += 4;							      \
diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
new file mode 100644
index 0000000000..8389adebf2
--- /dev/null
+++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
@@ -0,0 +1,160 @@
+/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734).
+   Copyright (C) 2020 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <string.h>
+#include <locale.h>
+#include <wchar.h>
+#include <support/check.h>
+#include <support/support.h>
+
+/* A few BIG5-HKSCS characters map in two unicode code points.
+   They are:
+   /x88/x62 => <U00CA><U0304>
+   /x88/x64 => <U00CA><U030C>
+   /x88/xa3 => <U00EA><U0304>
+   /x88/xa5 => <U00EA><U030C>
+   Each of these is special cased in iconvdata/big5hkscs.c.
+   This test ensures that we correctly reset the shift state after
+   outputting any of these characters.  We do this by converting
+   each them followed by converting an ASCII character.  If we fail
+   to reset the shift state (bug 25734) then we'll see the last
+   character in the queue output again.  */
+
+/* Each test has  name, input bytes, and expected wide character
+   output.  */
+struct testdata {
+  const char *name;
+  const char input[3];
+  wchar_t expected[3];
+};
+
+/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple
+   wide characters.  */
+struct testdata tests[4] = {
+  /* <H-8862>X => <U+00CA><U+0304>X */
+  { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } },
+  /* <H-8864>X => <U+00CA><U+030C>X */
+  { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } },
+  /* <H-88A3>X => <U+00EA><U+0304>X */
+  { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } },
+  /* <H-88A5>X => <U+00EA><U+030C>X */
+  { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } }
+};
+
+/* Each test is of the form:
+   - Translate first code sequence (two bytes)
+   - Translate second (zero bytes)
+   - Translate the third (one byte).  */
+static int
+check_conversion (struct testdata test)
+{
+  int err = 0;
+  wchar_t wc;
+  mbstate_t st;
+  size_t ret;
+  const char *mbs = test.input;
+  int consumed = 0;
+  /* Input is always 3 bytes long.  */
+  int inlen = 3;
+
+  memset (&st, 0, sizeof (st));
+  /* First conversion: Consumes first 2 bytes.  */
+  ret = mbrtowc (&wc, mbs, inlen - consumed, &st); 
+  if (ret != 2)
+    {
+      printf ("error: First conversion consumed only %zd bytes.\n", ret);
+      err++;
+    }
+  /* Advance the two consumed bytes.  */
+  mbs += ret;
+  consumed += ret;
+  if (wc != test.expected[0])
+    {
+      printf ("error: Result of first conversion was wrong.\n");
+      err++;
+    }
+  /* Second conversion: Consumes 0 bytes.  */
+  ret = mbrtowc (&wc, mbs, inlen - consumed, &st); 
+  if (ret != 0)
+    {
+      printf ("error: Second conversion consumed only %zd bytes.\n", ret);
+      err++;
+    }
+  /* Advance the zero consumed bytes.  */
+  mbs += ret;
+  consumed += ret;
+  if (wc != test.expected[1])
+    {
+      printf ("error: Result of second conversion was wrong.\n");
+      err++;
+    }
+  /* After the second conversion the state of the converter should be
+     in the initial state.  It is in the initial state because the two
+     input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have
+     been output.  */
+  if (mbsinit (&st) == 0)
+    {
+      printf ("error: Converter not in initial state.\n");
+      err++;
+    }
+  /* Third conversion: Consumes 1 byte (it's an ASCII character).  */
+  ret = mbrtowc (&wc, mbs, inlen - consumed, &st); 
+  if (ret != 1)
+    {
+      printf ("error: Third conversion consumed only %zd bytes.\n", ret);
+      err++;
+    }
+  /* Advance the one byte.  */
+  mbs += ret;
+  consumed += ret;
+  if (wc != test.expected[2])
+    {
+      printf ("error: Result of third conversion was wrong.\n");
+      err++;
+    }
+  /* Return 0 if we saw no errors.  */
+  return err;
+}
+
+static int
+do_test (void)
+{
+  int err = 0;
+  int ret;
+  /* Testing BIG5-HKSCS.  */
+  xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS");
+
+  /* Run all the special conversions.  */
+  for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++)
+    {
+      printf ("Running test for %s\n", tests[i].name);
+      ret = check_conversion (tests[i]);
+      if (ret > 0)
+	printf ("Test %s failed.\n", tests[i].name);
+      err += ret;
+    }
+
+  /* Fail if any conversion had an error.  */
+  if (err > 0)
+    FAIL_EXIT1 ("One or more conversions failed.");
+
+  return 0; 
+}
+
+#include <support/test-driver.c>
  

* Carlos O'Donell via Libc-alpha:

> An input BIG5-HKSCS character may be converted into at most 2 whcar_t

Typo: whcar_t
  

On 3/28/20 4:51 PM, Florian Weimer wrote:
> * Carlos O'Donell via Libc-alpha:
> 
>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
> 
> Typo: whcar_t
> 

Fixed. Thanks :-)
  

On Mär 27 2020, Carlos O'Donell wrote:

> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
> characters.

Could someone please file an interpretation request for POSIX, what
should happen in that two wchar_t case?  I think a case could be made
that mbrtowc should return -1/EILSEQ, but that also has implications for
mbsrtowcs, since that is defined in terms of repeated application of
mbrtowc.

Andreas.
  

* Andreas Schwab:

> On Mär 27 2020, Carlos O'Donell wrote:
>
>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
>> characters.
>
> Could someone please file an interpretation request for POSIX, what
> should happen in that two wchar_t case?

Why POSIX and not ISO C?

I'm not sure if the C committee wants implementations to be able to
support Big5 (without Unicode changes first, to add characters which
avoid the two-codepoint special cases).
  

On Mär 30 2020, Florian Weimer wrote:

> I'm not sure if the C committee wants implementations to be able to
> support Big5 (without Unicode changes first, to add characters which
> avoid the two-codepoint special cases).

Are you saying mbrtowc should return -1 here?

Andreas.
  

* Andreas Schwab:

> On Mär 30 2020, Florian Weimer wrote:
>
>> I'm not sure if the C committee wants implementations to be able to
>> support Big5 (without Unicode changes first, to add characters which
>> avoid the two-codepoint special cases).
>
> Are you saying mbrtowc should return -1 here?

Sorry, I haven't looked at the situation in detail, to determine what
the best possible choice in the context of our own implementation.

I merely don't think we can rely on C or POSIX to provide guidance how
to support locales with these fake shift states.
  

On 3/30/20 8:11 AM, Andreas Schwab wrote:
> On Mär 27 2020, Carlos O'Donell wrote:
> 
>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
>> characters.
> 
> Could someone please file an interpretation request for POSIX, what
> should happen in that two wchar_t case?  I think a case could be made
> that mbrtowc should return -1/EILSEQ, but that also has implications for
> mbsrtowcs, since that is defined in terms of repeated application of
> mbrtowc.

Tom had already reached out to ISO C WG14 to discuss this since there is
harmonization there between C and POSIX.

The current thinking in WG14 is that BIG5-HKSCS violates the definition
of "wide character" because the wchar_t value cannot represent the
original character in the locale and so the semantics underlying
BIG5-HKSCS will not map to the current API designs.

Florian raised a similar issue in May of 2019 and the general feedback
at that time was that BIG5-HKSCS is simply not supported by ISO C.
I expect the same answer from POSIX which is harmonized with ISO C in
this case.

If BIG5-HKSCS is not supported, then the standard will have nothing to
say about which values can be returned after the first or second input
bytes are read.

Options:

(a) Do not change the current converter. We return 2 consumed bytes in
    the first conversion, and 0 on the second.

(b) Look ahead and split the conversion. We return 1 consumed in the
    first conversion, and 1 on the second. This prevents us from returning
    0 which may be interpreted as a L'\0'. This leads to false assumption
    that the user could stop the conversion at this point and modify the
    input.

(c) Design something new. Return (size_t) -3 indicating a One-to-Many
    conversion is happening and that there is more output to be generated.

Do you still want me to file an interpretation request with POSIX?
  

On 3/30/20 10:28 AM, Andreas Schwab wrote:
> On Mär 30 2020, Florian Weimer wrote:
> 
>> I'm not sure if the C committee wants implementations to be able to
>> support Big5 (without Unicode changes first, to add characters which
>> avoid the two-codepoint special cases).
> 
> Are you saying mbrtowc should return -1 here?

No. That indicates an invalid multibyte sequence was found. The sequence
is not invalid. There are in fact no semantics that apply given the current
API design.
  

On Mär 30 2020, Carlos O'Donell via Libc-alpha wrote:

> On 3/30/20 10:28 AM, Andreas Schwab wrote:
>> On Mär 30 2020, Florian Weimer wrote:
>> 
>>> I'm not sure if the C committee wants implementations to be able to
>>> support Big5 (without Unicode changes first, to add characters which
>>> avoid the two-codepoint special cases).
>> 
>> Are you saying mbrtowc should return -1 here?
>
> No. That indicates an invalid multibyte sequence was found.

It is not representable, thus invalid.

Andreas.
  

On 3/30/20 10:34 AM, Carlos O'Donell wrote:
> On 3/30/20 8:11 AM, Andreas Schwab wrote:
>> On Mär 27 2020, Carlos O'Donell wrote:
>>
>>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
>>> characters.
>> Could someone please file an interpretation request for POSIX, what
>> should happen in that two wchar_t case?  I think a case could be made
>> that mbrtowc should return -1/EILSEQ, but that also has implications for
>> mbsrtowcs, since that is defined in terms of repeated application of
>> mbrtowc.
> Tom had already reached out to ISO C WG14 to discuss this since there is
> harmonization there between C and POSIX.
>
> The current thinking in WG14 is that BIG5-HKSCS violates the definition
> of "wide character" because the wchar_t value cannot represent the
> original character in the locale and so the semantics underlying
> BIG5-HKSCS will not map to the current API designs.
>
> Florian raised a similar issue in May of 2019 and the general feedback
> at that time was that BIG5-HKSCS is simply not supported by ISO C.
> I expect the same answer from POSIX which is harmonized with ISO C in
> this case.
>
> If BIG5-HKSCS is not supported, then the standard will have nothing to
> say about which values can be returned after the first or second input
> bytes are read.

If this is determined to be the case, then my preference is for option 
(c) below as this has the advantage of adopting behavior that is already 
standard for mbrtoc16.

Tom.

>
> Options:
>
> (a) Do not change the current converter. We return 2 consumed bytes in
>      the first conversion, and 0 on the second.
>
> (b) Look ahead and split the conversion. We return 1 consumed in the
>      first conversion, and 1 on the second. This prevents us from returning
>      0 which may be interpreted as a L'\0'. This leads to false assumption
>      that the user could stop the conversion at this point and modify the
>      input.
>
> (c) Design something new. Return (size_t) -3 indicating a One-to-Many
>      conversion is happening and that there is more output to be generated.
>
> Do you still want me to file an interpretation request with POSIX?
>
  

On 3/28/20 4:37 PM, Carlos O'Donell wrote:
> On 3/27/20 5:18 PM, Carlos O'Donell wrote:
>> Andreas,
>>
>> Does this look good to you?
>>
>> I admit it is odd that the converter returns 0-bytes consumed on the
>> second conversion, it could be enhanced to correctly show that we're
>> really consume 1-byte at a time, but that's another bug.
>>
> Tom,
>
> Thanks for your review!
>
> If you want you can give a "Reviewed-by:"

Thanks, Carlos!

Reviewed-by: Tom Honermann<tom@honermann.net>

Tom.

>
> Andreas,
>
> I'd like to see a review from you given your experience with BIG5.
>
> v2
> - Fixed typo.
> - Added mbsinit test (thanks Tom).
> - Fixed logical flaw in test loop in do_test.
>
>> 8< --- 8< --- 8< ---
>  From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001
> From: Carlos O'Donell <carlos@redhat.com>
> Date: Fri, 27 Mar 2020 17:03:36 -0400
> Subject: [PATCH] Reset converter state after second wchar_t output (Bug 25734)
>
> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
> characters. After outputting the second whcar_t character (which was
> saved in the converter state) we must reset the state. If we fail
> to reset the state we will be stuck continually copying that
> character to the output even if we have further input to consider.
>
> We add a new test case that covers the 4 BIG5-HKSCS characters
> that may become 2 wchar_t characters.
> ---
>   iconvdata/Makefile                        |   6 +-
>   iconvdata/big5hkscs.c                     |   3 +
>   iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++
>   3 files changed, 168 insertions(+), 1 deletion(-)
>   create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
>
> diff --git a/iconvdata/Makefile b/iconvdata/Makefile
> index c83962f351..71ddef09db 100644
> --- a/iconvdata/Makefile
> +++ b/iconvdata/Makefile
> @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules))
>   ifeq (yes,$(build-shared))
>   tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 bug-iconv4 \
>   	tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 bug-iconv9 \
> -	bug-iconv10 bug-iconv11 bug-iconv12
> +	bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4
>   ifeq ($(have-thread-library),yes)
>   tests += bug-iconv3
>   endif
> @@ -340,3 +340,7 @@ tst-tables-clean:
>   
>   $(objpfx)gconv-modules: gconv-modules
>   	cat $(sysdeps-gconv-modules) $^ > $@
> +
> +# Test requires BIG5HKSCS.
> +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \
> +			  $(addprefix $(objpfx),BIG5HKSCS.so)
> diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c
> index 01fcfeba76..ef325119b1 100644
> --- a/iconvdata/big5hkscs.c
> +++ b/iconvdata/big5hkscs.c
> @@ -17895,6 +17895,9 @@ static struct
>   	else								      \
>   	  ++inptr;							      \
>         }									      \
> +    else								      \
> +      /* Clear the queue and proceed to output the saved character.  */	      \
> +      *statep = 0;							      \
>   									      \
>       put32 (outptr, ch);							      \
>       outptr += 4;							      \
> diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
> new file mode 100644
> index 0000000000..8389adebf2
> --- /dev/null
> +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
> @@ -0,0 +1,160 @@
> +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734).
> +   Copyright (C) 2020 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <locale.h>
> +#include <wchar.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +
> +/* A few BIG5-HKSCS characters map in two unicode code points.
> +   They are:
> +   /x88/x62 => <U00CA><U0304>
> +   /x88/x64 => <U00CA><U030C>
> +   /x88/xa3 => <U00EA><U0304>
> +   /x88/xa5 => <U00EA><U030C>
> +   Each of these is special cased in iconvdata/big5hkscs.c.
> +   This test ensures that we correctly reset the shift state after
> +   outputting any of these characters.  We do this by converting
> +   each them followed by converting an ASCII character.  If we fail
> +   to reset the shift state (bug 25734) then we'll see the last
> +   character in the queue output again.  */
> +
> +/* Each test has  name, input bytes, and expected wide character
> +   output.  */
> +struct testdata {
> +  const char *name;
> +  const char input[3];
> +  wchar_t expected[3];
> +};
> +
> +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple
> +   wide characters.  */
> +struct testdata tests[4] = {
> +  /* <H-8862>X => <U+00CA><U+0304>X */
> +  { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } },
> +  /* <H-8864>X => <U+00CA><U+030C>X */
> +  { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } },
> +  /* <H-88A3>X => <U+00EA><U+0304>X */
> +  { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } },
> +  /* <H-88A5>X => <U+00EA><U+030C>X */
> +  { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } }
> +};
> +
> +/* Each test is of the form:
> +   - Translate first code sequence (two bytes)
> +   - Translate second (zero bytes)
> +   - Translate the third (one byte).  */
> +static int
> +check_conversion (struct testdata test)
> +{
> +  int err = 0;
> +  wchar_t wc;
> +  mbstate_t st;
> +  size_t ret;
> +  const char *mbs = test.input;
> +  int consumed = 0;
> +  /* Input is always 3 bytes long.  */
> +  int inlen = 3;
> +
> +  memset (&st, 0, sizeof (st));
> +  /* First conversion: Consumes first 2 bytes.  */
> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
> +  if (ret != 2)
> +    {
> +      printf ("error: First conversion consumed only %zd bytes.\n", ret);
> +      err++;
> +    }
> +  /* Advance the two consumed bytes.  */
> +  mbs += ret;
> +  consumed += ret;
> +  if (wc != test.expected[0])
> +    {
> +      printf ("error: Result of first conversion was wrong.\n");
> +      err++;
> +    }
> +  /* Second conversion: Consumes 0 bytes.  */
> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
> +  if (ret != 0)
> +    {
> +      printf ("error: Second conversion consumed only %zd bytes.\n", ret);
> +      err++;
> +    }
> +  /* Advance the zero consumed bytes.  */
> +  mbs += ret;
> +  consumed += ret;
> +  if (wc != test.expected[1])
> +    {
> +      printf ("error: Result of second conversion was wrong.\n");
> +      err++;
> +    }
> +  /* After the second conversion the state of the converter should be
> +     in the initial state.  It is in the initial state because the two
> +     input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have
> +     been output.  */
> +  if (mbsinit (&st) == 0)
> +    {
> +      printf ("error: Converter not in initial state.\n");
> +      err++;
> +    }
> +  /* Third conversion: Consumes 1 byte (it's an ASCII character).  */
> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
> +  if (ret != 1)
> +    {
> +      printf ("error: Third conversion consumed only %zd bytes.\n", ret);
> +      err++;
> +    }
> +  /* Advance the one byte.  */
> +  mbs += ret;
> +  consumed += ret;
> +  if (wc != test.expected[2])
> +    {
> +      printf ("error: Result of third conversion was wrong.\n");
> +      err++;
> +    }
> +  /* Return 0 if we saw no errors.  */
> +  return err;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  int err = 0;
> +  int ret;
> +  /* Testing BIG5-HKSCS.  */
> +  xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS");
> +
> +  /* Run all the special conversions.  */
> +  for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++)
> +    {
> +      printf ("Running test for %s\n", tests[i].name);
> +      ret = check_conversion (tests[i]);
> +      if (ret > 0)
> +	printf ("Test %s failed.\n", tests[i].name);
> +      err += ret;
> +    }
> +
> +  /* Fail if any conversion had an error.  */
> +  if (err > 0)
> +    FAIL_EXIT1 ("One or more conversions failed.");
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
  

On 3/30/20 11:28 AM, Andreas Schwab wrote:
> On Mär 30 2020, Carlos O'Donell via Libc-alpha wrote:
> 
>> On 3/30/20 10:28 AM, Andreas Schwab wrote:
>>> On Mär 30 2020, Florian Weimer wrote:
>>>
>>>> I'm not sure if the C committee wants implementations to be able to
>>>> support Big5 (without Unicode changes first, to add characters which
>>>> avoid the two-codepoint special cases).
>>>
>>> Are you saying mbrtowc should return -1 here?
>>
>> No. That indicates an invalid multibyte sequence was found.
> 
> It is not representable, thus invalid.

Sorry, I think I misunderstood your question.

I think you are actually asking what should a hypothetically correct
implementation do in this case?

If that is your question, then I agree, it should return -1 when it
finds any input that violates the C requirements.

I would *not* change glibc to do this though since BIG5-HKSCS is
supported and in use in glibc.

A simple converter can be written that goes through all input bytes
bytes until the input is at the end or errors out (rather than stopping
at the observed L'\0'), but it requires you know the length of the input.

I have seen many examples looking for result > 0 though, so I expect
such codes would immediately stop when encountering such BIG5-HKSCS
input that generates a 0 return.

The Microsoft docs have a similar example stopping the conversion when
0 is returned, but using -2 to continue stepping through the input,
advancing by one byte to attempt to put together the incomplete sequence
(expecting the state to accrue).

The Microsoft docs are here:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mbrtowc?view=vs-2019

Example here:
https://rextester.com/UYPGU65292

Windows:
1st mbrtowc call: 0xF325
  result: 2
2nd mbrtowc call: 0x0062
  result: 1
3rd mbrtowc call: 0x0058
  result: 1

Linux:
1st mbrtowc call: 0x00CA
  result: 2
2nd mbrtowc call: 0x0304
  result: 0
3rd mbrtowc call: 0x0058
  result: 1

Note that in the Microsoft implementation you *can't* use the
value of the return from mbrtowc to walk the input forward, and
that seems like a mistake to me, at least 0 is an honest (if
wrong) answer.
  

Carlos, I think this patch needs an additional update.  Comment inlined 
below.

On 3/30/20 1:40 PM, Tom Honermann wrote:
> On 3/28/20 4:37 PM, Carlos O'Donell wrote:
>> On 3/27/20 5:18 PM, Carlos O'Donell wrote:
>>> Andreas,
>>>
>>> Does this look good to you?
>>>
>>> I admit it is odd that the converter returns 0-bytes consumed on the
>>> second conversion, it could be enhanced to correctly show that we're
>>> really consume 1-byte at a time, but that's another bug.
>>>
>> Tom,
>>
>> Thanks for your review!
>>
>> If you want you can give a "Reviewed-by:"
>
> Thanks, Carlos!
>
> Reviewed-by: Tom Honermann<tom@honermann.net>
>
> Tom.
>
>>
>> Andreas,
>>
>> I'd like to see a review from you given your experience with BIG5.
>>
>> v2
>> - Fixed typo.
>> - Added mbsinit test (thanks Tom).
>> - Fixed logical flaw in test loop in do_test.
>>
>>> 8< --- 8< --- 8< ---
>>  From 40353453e97c321b20e683939c919e6d997fffcb Mon Sep 17 00:00:00 2001
>> From: Carlos O'Donell <carlos@redhat.com>
>> Date: Fri, 27 Mar 2020 17:03:36 -0400
>> Subject: [PATCH] Reset converter state after second wchar_t output 
>> (Bug 25734)
>>
>> An input BIG5-HKSCS character may be converted into at most 2 whcar_t
>> characters. After outputting the second whcar_t character (which was
>> saved in the converter state) we must reset the state. If we fail
>> to reset the state we will be stuck continually copying that
>> character to the output even if we have further input to consider.
>>
>> We add a new test case that covers the 4 BIG5-HKSCS characters
>> that may become 2 wchar_t characters.
>> ---
>>   iconvdata/Makefile                        |   6 +-
>>   iconvdata/big5hkscs.c                     |   3 +
>>   iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c | 160 ++++++++++++++++++++++
>>   3 files changed, 168 insertions(+), 1 deletion(-)
>>   create mode 100644 iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
>>
>> diff --git a/iconvdata/Makefile b/iconvdata/Makefile
>> index c83962f351..71ddef09db 100644
>> --- a/iconvdata/Makefile
>> +++ b/iconvdata/Makefile
>> @@ -73,7 +73,7 @@ modules.so := $(addsuffix .so, $(modules))
>>   ifeq (yes,$(build-shared))
>>   tests = bug-iconv1 bug-iconv2 tst-loading tst-e2big tst-iconv4 
>> bug-iconv4 \
>>       tst-iconv6 bug-iconv5 bug-iconv6 tst-iconv7 bug-iconv8 
>> bug-iconv9 \
>> -    bug-iconv10 bug-iconv11 bug-iconv12
>> +    bug-iconv10 bug-iconv11 bug-iconv12 tst-iconv-big5-hkscs-to-2ucs4
>>   ifeq ($(have-thread-library),yes)
>>   tests += bug-iconv3
>>   endif
>> @@ -340,3 +340,7 @@ tst-tables-clean:
>>     $(objpfx)gconv-modules: gconv-modules
>>       cat $(sysdeps-gconv-modules) $^ > $@
>> +
>> +# Test requires BIG5HKSCS.
>> +$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \
>> +              $(addprefix $(objpfx),BIG5HKSCS.so)
>> diff --git a/iconvdata/big5hkscs.c b/iconvdata/big5hkscs.c
>> index 01fcfeba76..ef325119b1 100644
>> --- a/iconvdata/big5hkscs.c
>> +++ b/iconvdata/big5hkscs.c
>> @@ -17895,6 +17895,9 @@ static struct
>>       else                                      \
>>         ++inptr;                                  \
>>         }                                          \
>> +    else                                      \
>> +      /* Clear the queue and proceed to output the saved character.  
>> */          \
>> +      *statep = 0;                                  \
>>                                             \
>>       put32 (outptr, ch);                                  \
>>       outptr += 4;                                  \
>> diff --git a/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c 
>> b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
>> new file mode 100644
>> index 0000000000..8389adebf2
>> --- /dev/null
>> +++ b/iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c
>> @@ -0,0 +1,160 @@
>> +/* Verify the BIG5HKSCS outputs that generate 2 wchar_t's (Bug 25734).
>> +   Copyright (C) 2020 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
>> +   <https://www.gnu.org/licenses/>.  */
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <locale.h>
>> +#include <wchar.h>
>> +#include <support/check.h>
>> +#include <support/support.h>
>> +
>> +/* A few BIG5-HKSCS characters map in two unicode code points.
>> +   They are:
>> +   /x88/x62 => <U00CA><U0304>
>> +   /x88/x64 => <U00CA><U030C>
>> +   /x88/xa3 => <U00EA><U0304>
>> +   /x88/xa5 => <U00EA><U030C>
>> +   Each of these is special cased in iconvdata/big5hkscs.c.
>> +   This test ensures that we correctly reset the shift state after
>> +   outputting any of these characters.  We do this by converting
>> +   each them followed by converting an ASCII character.  If we fail
>> +   to reset the shift state (bug 25734) then we'll see the last
>> +   character in the queue output again.  */
>> +
>> +/* Each test has  name, input bytes, and expected wide character
>> +   output.  */
>> +struct testdata {
>> +  const char *name;
>> +  const char input[3];
>> +  wchar_t expected[3];
>> +};
>> +
>> +/* In BIG5-HKSCS (2008) there are 4 characters that generate multiple
>> +   wide characters.  */
>> +struct testdata tests[4] = {
>> +  /* <H-8862>X => <U+00CA><U+0304>X */
>> +  { "<H-8862>", "\x88\x62\x58", { 0x00CA, 0x0304, 0x0058 } },
>> +  /* <H-8864>X => <U+00CA><U+030C>X */
>> +  { "<H-8864>", "\x88\x64\x58", { 0x00CA, 0x030C, 0x0058 } },
>> +  /* <H-88A3>X => <U+00EA><U+0304>X */
>> +  { "<H-88A3>", "\x88\xa3\x58", { 0x00EA, 0x0304, 0x0058 } },
>> +  /* <H-88A5>X => <U+00EA><U+030C>X */
>> +  { "<H-88A5>", "\x88\xa5\x58", { 0x00EA, 0x030C, 0x0058 } }
>> +};
>> +
>> +/* Each test is of the form:
>> +   - Translate first code sequence (two bytes)
>> +   - Translate second (zero bytes)
>> +   - Translate the third (one byte).  */
>> +static int
>> +check_conversion (struct testdata test)
>> +{
>> +  int err = 0;
>> +  wchar_t wc;
>> +  mbstate_t st;
>> +  size_t ret;
>> +  const char *mbs = test.input;
>> +  int consumed = 0;
>> +  /* Input is always 3 bytes long.  */
>> +  int inlen = 3;
>> +
>> +  memset (&st, 0, sizeof (st));
>> +  /* First conversion: Consumes first 2 bytes.  */
>> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
>> +  if (ret != 2)
>> +    {
>> +      printf ("error: First conversion consumed only %zd bytes.\n", 
>> ret);
>> +      err++;
>> +    }
>> +  /* Advance the two consumed bytes.  */
>> +  mbs += ret;
>> +  consumed += ret;
>> +  if (wc != test.expected[0])
>> +    {
>> +      printf ("error: Result of first conversion was wrong.\n");
>> +      err++;
>> +    }
>> +  /* Second conversion: Consumes 0 bytes.  */
>> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
>> +  if (ret != 0)
>> +    {
>> +      printf ("error: Second conversion consumed only %zd bytes.\n", 
>> ret);
>> +      err++;
>> +    }
>> +  /* Advance the zero consumed bytes.  */
>> +  mbs += ret;
>> +  consumed += ret;
>> +  if (wc != test.expected[1])
>> +    {
>> +      printf ("error: Result of second conversion was wrong.\n");
>> +      err++;
>> +    }
>> +  /* After the second conversion the state of the converter should be
>> +     in the initial state.  It is in the initial state because the two
>> +     input BIG5-HKSCS bytes have been consumed and the 2 wchar_t's have
>> +     been output.  */
>> +  if (mbsinit (&st) == 0)
>> +    {
>> +      printf ("error: Converter not in initial state.\n");
>> +      err++;
>> +    }
>> +  /* Third conversion: Consumes 1 byte (it's an ASCII character).  */
>> +  ret = mbrtowc (&wc, mbs, inlen - consumed, &st);
>> +  if (ret != 1)
>> +    {
>> +      printf ("error: Third conversion consumed only %zd bytes.\n", 
>> ret);
>> +      err++;
>> +    }
>> +  /* Advance the one byte.  */
>> +  mbs += ret;
>> +  consumed += ret;
>> +  if (wc != test.expected[2])
>> +    {
>> +      printf ("error: Result of third conversion was wrong.\n");
>> +      err++;
>> +    }
>> +  /* Return 0 if we saw no errors.  */
>> +  return err;
>> +}
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  int err = 0;
>> +  int ret;
>> +  /* Testing BIG5-HKSCS.  */
>> +  xsetlocale (LC_ALL, "zh_HK.BIG5-HKSCS");

Though my initial tests didn't reveal a problem, this call to 
xsetlocale() is now failing for me in a fresh environment.  The reason 
is that a locale for "zh_HK.BIG5-HKSCS" is not generated. For other work 
I've been doing, I had generated that locale, but the glibc testsuite 
doesn't automatically generate it today.

The following patch addresses this.  Note that the prior dependence on 
BIG5HKSCS.so was not sufficient (and is no longer needed) since this 
test really depends on the locale (not just the converter) being 
available.  The requirement for the locale may be sufficient reason to 
move this test under wcsmbs.  The iconvdata Makefile currently only 
requires generation of the de_DE.UTF-8 locale while the wcsmbs Makefile 
already requires many locales. And since this test exercises mbrtowc 
specifically, that might be a more appropriate place for it anyway.

diff --git a/iconvdata/Makefile b/iconvdata/Makefile
index 71ddef09db..3c0c5807b7 100644
--- a/iconvdata/Makefile
+++ b/iconvdata/Makefile
@@ -278,11 +278,12 @@ endif
  include ../Rules

  ifeq ($(run-built-tests),yes)
-LOCALES := de_DE.UTF-8
+LOCALES := de_DE.UTF-8 zh_HK.BIG5-HKSCS
  include ../gen-locales.mk

  $(objpfx)bug-iconv6.out: $(gen-locales)
  $(objpfx)tst-iconv7.out: $(gen-locales)
+$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(gen-locales)
  endif

  # Set libof-* for each routine.
@@ -340,7 +341,3 @@ tst-tables-clean:

  $(objpfx)gconv-modules: gconv-modules
         cat $(sysdeps-gconv-modules) $^ > $@
-
-# Test requires BIG5HKSCS.
-$(objpfx)tst-iconv-big5-hkscs-to-2ucs4.out: $(objpfx)gconv-modules \
-                         $(addprefix $(objpfx),BIG5HKSCS.so)

Tom.

>> +
>> +  /* Run all the special conversions.  */
>> +  for (int i = 0; i < (sizeof (tests) / sizeof (struct testdata)); i++)
>> +    {
>> +      printf ("Running test for %s\n", tests[i].name);
>> +      ret = check_conversion (tests[i]);
>> +      if (ret > 0)
>> +    printf ("Test %s failed.\n", tests[i].name);
>> +      err += ret;
>> +    }
>> +
>> +  /* Fail if any conversion had an error.  */
>> +  if (err > 0)
>> +    FAIL_EXIT1 ("One or more conversions failed.");
>> +
>> +  return 0;
>> +}
>> +
>> +#include <support/test-driver.c>
>
>