Subject: Re: t3900 failure on macOS, iconv(3) broken?

From: René Scharfe <[email-masked]>
Date: Tue, 9 Dec 2025 23:25:32 +0100

On 12/9/25 10:24 PM, Torsten Bögershausen wrote:
> On Tue, Dec 09, 2025 at 08:35:23PM +0100, René Scharfe wrote:
>> On 12/9/25 5:33 PM, Torsten Bögershausen wrote:
>>> On Mon, Dec 08, 2025 at 11:59:11PM +0100, René Scharfe wrote:
>>>>
> [snip]
>>
>> This forgets to reset outsz and the converter state.  With this patch
>> t0028-working-tree-encoding.sh seems to get stuck in an endless loop.
> 
> Thanks for testing.
> I did another test here
> (increase the outbuffer with only one byte per round, old MacOs)
> and yes, we need to reset iconv.
> Back to your patch. I think it is good to go further,
> with one or 2 remarks, see TB
>  
> 			out = xrealloc(out, outalloc);
> 			// TB: move into else outpos = out + sofar;
> 			// TB: move into else outsz = outalloc - sofar - 1;
> // TB: We have seen different breakages of apple iconv. Should we run the same code
> // on all versions of MacOs to be more future proof ?
> // and do we need a Makefile knob, if one, and only one platform is affected ?
> // I don't know
> #ifdef __APPLE__
> or
> #ifdef ICONV_BREAKS

macOS 14.8.2 reportedly doesn't have this particular issue, and I can
only hope that Apple will eventually fix that bug, so __APPLE__ seems a
bit too broad.

I'm also not thrilled about adding yet another build flag.  The patch I
just posted sidesteps the issue by using the existing ICONVDIR setting
to use libiconv from Homebrew.  We do that for gettext already, so it
should be fine..

> 			/*
> 			 * If iconv(3) messes up piecemeal conversions
> 			 * then restore the original pointers, sizes,
> 			 * and converter state, then retry converting
> 			 * the full string using the reallocated buffer.
> 			 */
> 			insz += (char *)cp - in;    /* TB stumbled here: "in" is "const char *"

We can add the const qualifier, but it won't affect the pointer
arithmetic.  Perhaps casting to iconv_ibp would be more consistent?

> 			                              And I didn't like the fact that insz is destroyed
> 						      and needs to be restored. That is why I had a originsz
> 						      (or szinorig ?)

Sure, storing the original value would work, but is slightly more effort
than subtracting the progress made so far.  originsz would only be used
if ICONV_BREAKS is defined, you'd need to declare it conditionally,
adding yet more overhead.

> 			cp = (iconv_ibp)in;
> 			outpos = out + bom_len;
> 			outsz = outalloc - bom_len - 1;
> 			iconv(conv, NULL, NULL, NULL, NULL);
> #else
> 			outpos = out + sofar;
> 			outsz = outalloc - sofar - 1;

I'd like to keep buffer increase and rollback separate.  Perhaps
splitting out the output buffer adjustment is worth it, though?  Not
sure. *shrug*

diff --git a/utf8.c b/utf8.c
index 35a0251939..c99243a63b 100644
--- a/utf8.c
+++ b/utf8.c
@@ -513,6 +513,18 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv,
 			sofar = outpos - out;
 			outalloc = st_add3(sofar, st_mult(insz, 2), 32);
 			out = xrealloc(out, outalloc);
+#ifdef ICONV_BREAKS
+			/*
+			 * If iconv(3) messes up piecemeal conversions
+			 * then restore the original pointers, sizes,
+			 * and converter state, then retry converting
+			 * the full string using the reallocated buffer.
+			 */
+			insz += cp - (iconv_ibp)in;
+			cp = (iconv_ibp)in;
+			sofar = bom_len;
+			iconv(conv, NULL, NULL, NULL, NULL);
+#endif
 			outpos = out + sofar;
 			outsz = outalloc - sofar - 1;
 		}

> #endif