iconv: Preserve iconv -c error exit on invalid inputs (bug 32046)

In several converters, a __GCONV_ILLEGAL_INPUT result gets overwritten
with __GCONV_FULL_OUTPUT.  As a result, iconv (the function) returns
E2BIG instead of EILSEQ.  The iconv program does not see the original
EILSEQ failure, does not recognize the invalid input, and may
incorrectly exit successfully.

To address this, a new __flags bit is used to indicate a sticky input
error state.  All __GCONV_ILLEGAL_INPUT results are replaced with a
function call that sets this new __GCONV_ENCOUNTERED_ILLEGAL_INPUT and
returns __GCONV_ILLEGAL_INPUT.  The iconv program checks for
__GCONV_ENCOUNTERED_ILLEGAL_INPUT and overrides the exit status.

The converter changes introducing __gconv_mark_illegal_input are
mostly mechanical, except for the res variable initialization in
iconvdata/iso-2022-jp.c: this error gets overwritten with __GCONV_OK
and other results in the following code.  If res ==
__GCONV_ILLEGAL_INPUT afterwards, STANDARD_TO_LOOP_ERR_HANDLER below
will handle it.

The __gconv_mark_illegal_input changes do not alter the errno value
set by the iconv function.  This is simpler to implement than
reviewing each __GCONV_FULL_OUTPUT result and adjust it not to
override a previous __GCONV_ILLEGAL_INPUT result.  Doing it that way
would also change some E2BIG errors in to EILSEQ errors, so it had to
be done conditionally (under a flag set by the iconv program only), to
avoid confusing buffer management in other applications.

Reviewed-by: DJ Delorie <dj@redhat.com>

8 files changed, 187 insertions, 15 deletions

diff --git a/iconv/Makefile b/iconv/Makefile
index b0fa550141..29e4f280ec 100644
--- a/iconv/Makefile
+++ b/iconv/Makefile
@@ -61,6 +61,10 @@ test-srcs := \
 	tst-translit-mchar \
 	# test-srcs
 
+tests-internal = \
+	tst-iconv-sticky-input-error \
+	# tests-internal
+
 others		= iconv_prog iconvconfig
 install-others-programs	= $(inst_bindir)/iconv
 install-sbin	= iconvconfig
diff --git a/iconv/gconv_int.h b/iconv/gconv_int.h
index 9fece3ea14..cd452d94cc 100644
--- a/iconv/gconv_int.h
+++ b/iconv/gconv_int.h
@@ -331,4 +331,34 @@ extern wint_t __gconv_btwoc_ascii (struct __gconv_step *step, unsigned char c);
 
 __END_DECLS
 
+/* Internal extensions for <gconv.h>.  */
+
+/* Internal flags for __flags in struct __gconv_step_data.  Overlaps
+   with flags for __gconv_open.  */
+enum
+  {
+    /* The conversion encountered an illegal input character at one
+       point.  */
+    __GCONV_ENCOUNTERED_ILLEGAL_INPUT = 1U << 30,
+  };
+
+/* Mark *STEP_DATA as having seen illegal input, and return
+   __GCONV_ILLEGAL_INPUT.  */
+static inline int
+__gconv_mark_illegal_input (struct __gconv_step_data *step_data)
+{
+  step_data->__flags |= __GCONV_ENCOUNTERED_ILLEGAL_INPUT;
+  return __GCONV_ILLEGAL_INPUT;
+}
+
+/* Returns true if any of the conversion steps encountered illegal input.  */
+static _Bool __attribute__ ((unused))
+__gconv_has_illegal_input (__gconv_t cd)
+{
+  for (size_t i = 0; i < cd->__nsteps; ++i)
+    if (cd->__data[i].__flags & __GCONV_ENCOUNTERED_ILLEGAL_INPUT)
+      return true;
+  return false;
+}
+
 #endif /* gconv_int.h */
diff --git a/iconv/gconv_simple.c b/iconv/gconv_simple.c
index 257be2f8ff..f22002cf81 100644
--- a/iconv/gconv_simple.c
+++ b/iconv/gconv_simple.c
@@ -207,7 +207,7 @@ ucs4_internal_loop (struct __gconv_step *step,
 	     UCS4 does not allow such values.  */
 	  if (irreversible == NULL)
 	    /* We are transliterating, don't try to correct anything.  */
-	    return __GCONV_ILLEGAL_INPUT;
+	    return __gconv_mark_illegal_input (step_data);
 
 	  if (flags & __GCONV_IGNORE_ERRORS)
 	    {
@@ -218,7 +218,7 @@ ucs4_internal_loop (struct __gconv_step *step,
 
 	  *inptrp = inptr;
 	  *outptrp = outptr;
-	  return __GCONV_ILLEGAL_INPUT;
+	  return __gconv_mark_illegal_input (step_data);
 	}
 
       put32 (outptr, inval);
@@ -276,7 +276,7 @@ ucs4_internal_loop_single (struct __gconv_step *step,
       if (!(flags & __GCONV_IGNORE_ERRORS))
 	{
 	  *inptrp -= cnt - (state->__count & 7);
-	  return __GCONV_ILLEGAL_INPUT;
+	  return __gconv_mark_illegal_input (step_data);
 	}
     }
   else
@@ -453,7 +453,7 @@ ucs4le_internal_loop (struct __gconv_step *step,
 	     UCS4 does not allow such values.  */
 	  if (irreversible == NULL)
 	    /* We are transliterating, don't try to correct anything.  */
-	    return __GCONV_ILLEGAL_INPUT;
+	    return __gconv_mark_illegal_input (step_data);
 
 	  if (flags & __GCONV_IGNORE_ERRORS)
 	    {
@@ -464,7 +464,7 @@ ucs4le_internal_loop (struct __gconv_step *step,
 
 	  *inptrp = inptr;
 	  *outptrp = outptr;
-	  return __GCONV_ILLEGAL_INPUT;
+	  return __gconv_mark_illegal_input (step_data);
 	}
 
       put32 (outptr, inval);
@@ -523,7 +523,7 @@ ucs4le_internal_loop_single (struct __gconv_step *step,
 	 represent the result.  This is a genuine bug in the input since
 	 UCS4 does not allow such values.  */
       if (!(flags & __GCONV_IGNORE_ERRORS))
-	return __GCONV_ILLEGAL_INPUT;
+	return __gconv_mark_illegal_input (step_data);
     }
   else
     {
@@ -969,7 +969,7 @@ ucs4le_internal_loop_single (struct __gconv_step *step,
 	   surrogates pass through, attackers could make a security	      \
 	   hole exploit by synthesizing any desired plane 1-16		      \
 	   character.  */						      \
-	result = __GCONV_ILLEGAL_INPUT;					      \
+	result = __gconv_mark_illegal_input (step_data);		      \
 	if (! ignore_errors_p ())					      \
 	  break;							      \
 	inptr += 4;							      \
@@ -1012,7 +1012,7 @@ ucs4le_internal_loop_single (struct __gconv_step *step,
 	   them.  (Catching this here is not security relevant.)  */	      \
 	if (! ignore_errors_p ())					      \
 	  {								      \
-	    result = __GCONV_ILLEGAL_INPUT;				      \
+	    result = __gconv_mark_illegal_input (step_data);		      \
 	    break;							      \
 	  }								      \
 	inptr += 2;							      \
@@ -1061,7 +1061,7 @@ ucs4le_internal_loop_single (struct __gconv_step *step,
 	   character.  */						      \
 	if (! ignore_errors_p ())					      \
 	  {								      \
-	    result = __GCONV_ILLEGAL_INPUT;				      \
+	    result = __gconv_mark_illegal_input (step_data);		      \
 	    break;							      \
 	  }								      \
 	inptr += 4;							      \
diff --git a/iconv/gconv_trans.c b/iconv/gconv_trans.c
index 44f0fd849a..54c4f3a100 100644
--- a/iconv/gconv_trans.c
+++ b/iconv/gconv_trans.c
@@ -232,6 +232,6 @@ __gconv_transliterate (struct __gconv_step *step,
     }
 
   /* Haven't found a match.  */
-  return __GCONV_ILLEGAL_INPUT;
+  return __gconv_mark_illegal_input (step_data);
 }
 libc_hidden_def (__gconv_transliterate)
diff --git a/iconv/iconv_prog.c b/iconv/iconv_prog.c
index 88a928557e..5fe4fe7a6c 100644
--- a/iconv/iconv_prog.c
+++ b/iconv/iconv_prog.c
@@ -291,6 +291,11 @@ conversions from `%s' and to `%s' are not supported"),
 	  }
 	while (++remaining < argc);
 
+      /* Ensure that iconv -c still exits with failure if iconv (the
+	 function) has failed with E2BIG instead of EILSEQ.  */
+      if (__gconv_has_illegal_input (cd))
+	status = EXIT_FAILURE;
+
       /* Close the output file now.  */
       if (output != NULL && fclose (output))
 	error (EXIT_FAILURE, errno, _("error while closing output file"));
diff --git a/iconv/loop.c b/iconv/loop.c
index 5340dafc70..199fb28326 100644
--- a/iconv/loop.c
+++ b/iconv/loop.c
@@ -123,8 +123,7 @@
    `continue' must reach certain points.  */
 #define STANDARD_FROM_LOOP_ERR_HANDLER(Incr) \
   {									      \
-    result = __GCONV_ILLEGAL_INPUT;					      \
-									      \
+    result = __gconv_mark_illegal_input (step_data);			      \
     if (! ignore_errors_p ())						      \
       break;								      \
 									      \
@@ -142,7 +141,7 @@
    points.  */
 #define STANDARD_TO_LOOP_ERR_HANDLER(Incr) \
   {									      \
-    result = __GCONV_ILLEGAL_INPUT;					      \
+    result = __gconv_mark_illegal_input (step_data);			      \
 									      \
     if (irreversible == NULL)						      \
       /* This means we are in call from __gconv_transliterate.  In this	      \
diff --git a/iconv/tst-iconv-sticky-input-error.c b/iconv/tst-iconv-sticky-input-error.c
new file mode 100644
index 0000000000..34a245f185
--- /dev/null
+++ b/iconv/tst-iconv-sticky-input-error.c
@@ -0,0 +1,135 @@
+/* Test __GCONV_ENCOUNTERED_ILLEGAL_INPUT, as used by iconv -c (bug 32046).
+   Copyright (C) 2024 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 <array_length.h>
+#include <errno.h>
+#include <gconv_int.h>
+#include <iconv.h>
+#include <stdbool.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <stdio.h>
+
+/* FROM is the input character set, TO the output character set.  If
+   IGNORE is true, the iconv descriptor is set up in the same way as
+   iconv -c would.  INPUT is the input string, EXPECTED_OUTPUT the
+   output.  OUTPUT_LIMIT is a byte count, specifying how many input
+   bytes are passed to the iconv function on each invocation.  */
+static void
+one_direction (const char *from, const char *to, bool ignore,
+               const char *input, const char *expected_output,
+               size_t output_limit)
+{
+  if (test_verbose)
+    {
+      char *quoted_input = support_quote_string (input);
+      char *quoted_output = support_quote_string (expected_output);
+      printf ("info: testing from=\"%s\" to=\"%s\" ignore=%d input=\"%s\""
+              " expected_output=\"%s\" output_limit=%zu\n",
+              from, to, (int) ignore, quoted_input,
+              quoted_output, output_limit);
+      free (quoted_output);
+      free (quoted_input);
+    }
+
+  __gconv_t cd;
+  if (ignore)
+    {
+      struct gconv_spec conv_spec;
+      TEST_VERIFY_EXIT (__gconv_create_spec (&conv_spec, from, to)
+                        == &conv_spec);
+      conv_spec.ignore = true;
+      cd = (iconv_t) -1;
+      TEST_COMPARE (__gconv_open (&conv_spec, &cd, 0), __GCONV_OK);
+      __gconv_destroy_spec (&conv_spec);
+    }
+  else
+    cd = iconv_open (to, from);
+  TEST_VERIFY_EXIT (cd != (iconv_t) -1);
+
+  char *input_ptr = (char *) input;
+  size_t input_len = strlen (input);
+  char output_buf[20];
+  char *output_ptr = output_buf;
+  size_t output_len;
+  do
+    {
+      output_len = array_end (output_buf) - output_ptr;
+      if (output_len > output_limit)
+        /* Limit the buffer size as requested by the caller.  */
+        output_len = output_limit;
+      TEST_VERIFY_EXIT (output_len > 0);
+      if (input_len == 0)
+        /* Trigger final flush.  */
+        input_ptr = NULL;
+      char *old_input_ptr = input_ptr;
+      size_t ret = iconv (cd, &input_ptr, &input_len,
+                          &output_ptr, &output_len);
+      if (ret == (size_t) -1)
+        {
+          if (errno != EILSEQ)
+            TEST_COMPARE (errno, E2BIG);
+        }
+
+      if (input_ptr == old_input_ptr)
+        /* Avoid endless loop if stuck on an invalid input character.  */
+        break;
+    }
+  while (input_ptr != NULL);
+
+  /* Test the sticky illegal input bit.  */
+  TEST_VERIFY (__gconv_has_illegal_input (cd));
+
+  TEST_COMPARE_BLOB (expected_output, strlen (expected_output),
+                     output_buf, output_ptr - output_buf);
+
+  TEST_COMPARE (iconv_close (cd), 0);
+}
+
+static int
+do_test (void)
+{
+  static const char charsets[][14] =
+    {
+      "ASCII",
+      "ASCII//IGNORE",
+      "UTF-8",
+      "UTF-8//IGNORE",
+    };
+
+  for (size_t from_idx = 0; from_idx < array_length (charsets); ++from_idx)
+    for (size_t to_idx = 0; to_idx < array_length (charsets); ++to_idx)
+      for (int do_ignore = 0; do_ignore < 2; ++do_ignore)
+        for (int limit = 1; limit < 5; ++limit)
+          for (int skip = 0; skip < 3; ++skip)
+            {
+              const char *expected_output;
+              if (do_ignore || strstr (charsets[to_idx], "//IGNORE") != NULL)
+                expected_output = "ABXY" + skip;
+              else
+                expected_output = "AB" + skip;
+              one_direction (charsets[from_idx], charsets[to_idx], do_ignore,
+                             "AB\xffXY" + skip, expected_output, limit);
+            }
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/iconv/tst-iconv_prog-buffer.sh b/iconv/tst-iconv_prog-buffer.sh
index a27107f02b..5ff99a02a3 100644
--- a/iconv/tst-iconv_prog-buffer.sh
+++ b/iconv/tst-iconv_prog-buffer.sh
@@ -150,9 +150,8 @@ expect_exit 1 \
 ! test -s "$tmp/err"
 expect_files abc def
 
-# FIXME: This is not correct, -c should not change the exit status.
 cp "$tmp/out-template" "$tmp/out"
-run_iconv -c -o "$tmp/out" \
+expect_exit 1 run_iconv -c -o "$tmp/out" \
     "$tmp/abc" "$tmp/0xff-wrapped" "$tmp/def" 2>"$tmp/err"
 ! test -s "$tmp/err"
 expect_files abc xy zt def