Re: [PATCH] xfsdump: avoid false-positive __strcpy_chk abort on long dirent names

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jun 25, 2026 at 03:23:37PM -0700, vmfunc wrote:
> direnthdr_t and direnthdr_v1_t end in dh_name[6], which is used as a
> trailing variable-length buffer rather than a fixed six-byte field.
> dump_dirent() allocates each record with malloc(sz), where
> 
> 	sz = offsetofmember(direnthdr*_t, dh_name) + namelen + 1
> 
> (rounded up to DIRENTHDR_ALIGN), and the "sz > direntbufsz" check ahead
> of the copy guarantees the allocation is large enough to hold the name.
> The declared [6] is part of the on-media record layout: DIRENTHDR_SZ and
> the arch translation paths depend on it, so it cannot simply be turned
> into a [] flexible array member.
> 
> When xfsdump is built with _FORTIFY_SOURCE together with
> -fstrict-flex-arrays (increasingly the default in distributions), the
> trailing dh_name[6] is no longer treated as a flexible array.
> __builtin_object_size() sizes it to 6, strcpy() is rewritten to
> __strcpy_chk(dst, src, 6), and dumping any directory entry whose name is
> longer than five bytes aborts at runtime:
> 
> 	*** buffer overflow detected ***: terminated
> 	#4  __fortify_fail
> 	#5  __chk_fail
> 	#6  __strcpy_chk
> 	#7  dump_dirent
> 
> The copy is in fact safe; only fortify's notion of the destination size
> is wrong. Write the name through the byte offset dump_dirent() already
> computes, (char *)dhdrp + name_offset, instead of through the sized
> dh_name lvalue. The destination is then a plain char * into the malloc'd
> record, which fortify cannot sub-object-size, so the spurious check is
> dropped while the rest of the binary keeps its hardening.
> 
> Signed-off-by: vmfunc <celeste@collar.sh>
> ---
> Andrey Albershteyn (Cc'd) tested this with fstests on the downstream
> report and asked that it be sent upstream; the discussion and original
> backtrace are at https://github.com/NixOS/nixpkgs/pull/533325 .
> 
>  dump/content.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/dump/content.c b/dump/content.c
> index 6462267..8d482cc 100644
> --- a/dump/content.c
> +++ b/dump/content.c
> @@ -5151,14 +5151,14 @@ dump_dirent(drive_t *drivep,
>  		dhdrp->dh_sz = (uint16_t)sz;
>  		dhdrp->dh_gen = (uint16_t)(gen & DENTGENMASK);
>  		if (name) {
> -			strcpy(dhdrp->dh_name, name);
> +			strcpy((char *)dhdrp + name_offset, name);
>  		}
>  
>  		dhdrp->dh_checksum = calc_checksum(dhdrp, DIRENTHDR_SZ);
>  
>  		xlate_direnthdr_v1(dhdrp, tmpdhdrp, 1);
>  		if (name) {
> -			strcpy(tmpdhdrp->dh_name, name);
> +			strcpy((char *)tmpdhdrp + name_offset, name);

Why wouldn't you fix the structure definitions to use the proper VLA
syntax (dh_name[]) instead of making the code less readable?

--D

>  		}
>  	} else {
>  		direnthdr_t *dhdrp = (direnthdr_t *)contextp->cc_mdirentbufp;
> @@ -5169,14 +5169,14 @@ dump_dirent(drive_t *drivep,
>  		dhdrp->dh_gen = gen;
>  		dhdrp->dh_sz = (uint16_t)sz;
>  		if (name) {
> -			strcpy(dhdrp->dh_name, name);
> +			strcpy((char *)dhdrp + name_offset, name);
>  		}
>  
>  		dhdrp->dh_checksum = calc_checksum(dhdrp, DIRENTHDR_SZ);
>  
>  		xlate_direnthdr(dhdrp, tmpdhdrp, 1);
>  		if (name) {
> -			strcpy(tmpdhdrp->dh_name, name);
> +			strcpy((char *)tmpdhdrp + name_offset, name);
>  		}
>  	}
>  
> -- 
> 2.54.0
> 
>