Re: t3900 failure on macOS, iconv(3) broken?
- From
René Scharfe <l.s.r@web.de>
- Date
- Dec 9, 2025, 22:25 UTC
- Message-ID
- <22b1f482-2012-4ee9-bc12-1b2123ee0101@web.de>
- In-Reply-To
- <20251209212420.GA10149@tb-raspi4>
On 12/9/25 10:24 PM, Torsten Bögershausen wrote:
Show 26 quoted lines
> 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..
Show 7 quoted lines
> /* > * 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.
Show 7 quoted lines
> 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