From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 03 Jul 2026 08:40:59 GMT
Subject: Re: [PATCH] precompose_utf8: use a flex array for d_name
Message-ID: <akd1m6KoUh7N8yyE@pks.im>
In-Reply-To: <20260703023554.36577-1-ihar.hrachyshka@gmail.com>

On Thu, Jul 02, 2026 at 10:35:54PM -0400, Ihar Hrachyshka wrote:
> On macOS, git status may abort while reading a directory entry
> whose UTF-8 name grows past NAME_MAX bytes:
> 
>   __chk_fail_overflow
>   __strlcpy_chk
>   precompose_utf8_readdir
>   read_directory_recursive
>   wt_status_collect
>   cmd_status
> 
> The precompose wrapper already reallocates dirent_prec_psx for
> long names, but d_name is declared as char[NAME_MAX + 1]. A
> fortified libc can still see that declared object size and reject a
> larger strlcpy bound, even though the allocation was grown.
> 
> Make d_name a FLEX_ARRAY and size allocations from offsetof(). That
> matches the actual object layout with the dynamic allocation, so the
> fortified copy sees a destination whose size can grow with max_name_len.
> 
> Add a regression test that creates a 261-byte non-ASCII basename and
> runs status with core.precomposeunicode enabled.

Hm. Why does macOS even allow you to create a file that has a basename
longer than NAME_MAX? Does macOS count unicode characters specially?

> diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
> index 1711794..8077f62 100644
> --- a/compat/precompose_utf8.c
> +++ b/compat/precompose_utf8.c
> @@ -19,6 +19,11 @@ typedef char *iconv_ibp;
>  static const char *repo_encoding = "UTF-8";
>  static const char *path_encoding = "UTF-8-MAC";
>  
> +static size_t dirent_prec_psx_size(size_t max_name_len)
> +{
> +	return st_add(offsetof(dirent_prec_psx, d_name), max_name_len);
> +}
> +
>  static size_t has_non_ascii(const char *s, size_t maxlen, size_t *strlen_c)
>  {
>  	const uint8_t *ptr = (const uint8_t *)s;
> @@ -114,8 +119,8 @@ const char *precompose_argv_prefix(int argc, const char **argv, const char *pref
>  PREC_DIR *precompose_utf8_opendir(const char *dirname)
>  {
>  	PREC_DIR *prec_dir = xmalloc(sizeof(PREC_DIR));
> -	prec_dir->dirent_nfc = xmalloc(sizeof(dirent_prec_psx));
> -	prec_dir->dirent_nfc->max_name_len = sizeof(prec_dir->dirent_nfc->d_name);
> +	prec_dir->dirent_nfc = xmalloc(dirent_prec_psx_size(NAME_MAX + 1));
> +	prec_dir->dirent_nfc->max_name_len = NAME_MAX + 1;

We have the `FLEX_ALLOC_MEM()` macro that would probably be a better fit
compared to introducing `dirent_prec_psx_size()`.

Also, when converting this to a flex array, can't we do better here and
allocate the structures with the right size? Otherwise, I expect that we
overallocate most of the entrise.

> @@ -145,8 +150,7 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir)
>  		int ret_errno = errno;
>  
>  		if (new_maxlen > prec_dir->dirent_nfc->max_name_len) {
> -			size_t new_len = sizeof(dirent_prec_psx) + new_maxlen -
> -				sizeof(prec_dir->dirent_nfc->d_name);
> +			size_t new_len = dirent_prec_psx_size(new_maxlen);
>  
>  			prec_dir->dirent_nfc = xrealloc(prec_dir->dirent_nfc, new_len);
>  			prec_dir->dirent_nfc->max_name_len = new_maxlen;

Okay, here we indeed have to realloc though, and thus we can't quite
avoid `dirent_prec_psx_size()`. Too bad.

Thanks!

Patrick