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

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

[vmfunc]

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);
 		}
 	} 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

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

[Darrick J. Wong]

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
> 
> 

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

[vmfunc]

On Thu, Jun 25, 2026 at 03:57:58PM -0700, Darrick J. Wong wrote:
> Why wouldn't you fix the structure definitions to use the proper VLA
> syntax (dh_name[]) instead of making the code less readable?

looked at that first but dh_name[] doesnt drop in cleanly here. the
declared size isnt slack padding, its the count of name bytes carried
inline in the fixed-size on-media header, n read_dirent() in
restore/content.c reads the record back out using sizeof() of that member:

	namep = dhdrp->dh_name + sizeof(dhdrp->dh_name);
	memcpy(dhdrp->dh_name, dhdr_v1.dh_name, sizeof(dhdr_v1.dh_name));

sizeof() on a flex array member is a constraint violation, so dh_name[]
wont compile without rewriting that reader. it also breaks
assert(sizeof(direnthdr_v1_t) == DIRENTHDR_SZ): the v1 header is 16 bytes
of fields n dh_name[8] is exactly the pad that brings it up to
DIRENTHDR_SZ (24). as a [] member it goes to 16 n the record size moves
with it.

so a proper [] conversion means reworking the v1 on-media reader + the
record-size invariants, felt out of proportion for fixing the fortify
abort, so i kept this to the minimal change that preserves the layout n
the rest of the hardening. happy to do the struct cleanup as a separate
patch if u'd rather, just flagging its a format change not a pure
readability one.

thanks,
celeste

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

[Christoph Hellwig]

On Fri, Jun 26, 2026 at 12:28:27AM +0000, vmfunc wrote:
> On Thu, Jun 25, 2026 at 03:57:58PM -0700, Darrick J. Wong wrote:
> > Why wouldn't you fix the structure definitions to use the proper VLA
> > syntax (dh_name[]) instead of making the code less readable?
> 
> looked at that first but dh_name[] doesnt drop in cleanly here. the
> declared size isnt slack padding, its the count of name bytes carried
> inline in the fixed-size on-media header, n read_dirent() in
> restore/content.c reads the record back out using sizeof() of that member:
> 
> 	namep = dhdrp->dh_name + sizeof(dhdrp->dh_name);
> 	memcpy(dhdrp->dh_name, dhdr_v1.dh_name, sizeof(dhdr_v1.dh_name));

To me it looks like the problem is that mix up the on-disk and
in-memory structure with different semantics here.  The somewhat
more invasive fix would be to split the

direnthdr structure into an in-memory direnthdr one, and an on-disk
direnthdr_v3.  The former would use a VLA, and the latter the existing
hard coded value.  This would also allow to make the endian conversion
in xlate_direnthdr/xlate_direnthdr_v1 type safe.

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

[Andrey Albershteyn]

On 2026-06-26 00:28:26, vmfunc wrote:
> On Thu, Jun 25, 2026 at 03:57:58PM -0700, Darrick J. Wong wrote:
> > Why wouldn't you fix the structure definitions to use the proper VLA
> > syntax (dh_name[]) instead of making the code less readable?
> 
> looked at that first but dh_name[] doesnt drop in cleanly here. the
> declared size isnt slack padding, its the count of name bytes carried
> inline in the fixed-size on-media header, n read_dirent() in
> restore/content.c reads the record back out using sizeof() of that member:
> 
> 	namep = dhdrp->dh_name + sizeof(dhdrp->dh_name);
> 	memcpy(dhdrp->dh_name, dhdr_v1.dh_name, sizeof(dhdr_v1.dh_name));
> 
> sizeof() on a flex array member is a constraint violation, so dh_name[]
> wont compile without rewriting that reader. it also breaks
> assert(sizeof(direnthdr_v1_t) == DIRENTHDR_SZ): the v1 header is 16 bytes
> of fields n dh_name[8] is exactly the pad that brings it up to
> DIRENTHDR_SZ (24). as a [] member it goes to 16 n the record size moves
> with it.
> 
> so a proper [] conversion means reworking the v1 on-media reader + the
> record-size invariants, felt out of proportion for fixing the fortify
> abort, so i kept this to the minimal change that preserves the layout n
> the rest of the hardening. happy to do the struct cleanup as a separate
> patch if u'd rather, just flagging its a format change not a pure
> readability one.
> 
> thanks,
> celeste
> 

My thoughts too, seems like easy enough fix to make fortified
version not crash.

-- 
- Andrey

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

[Celeste]

should we then assume that this is fine to merge?

- vmfunc

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

[Christoph Hellwig]

On Thu, Jul 02, 2026 at 12:58:14PM -0700, Celeste wrote:
> should we then assume that this is fine to merge?

I still don't think that just papering over the misuse of the same type
for on-disk and in-memory uses with different characteristics is a good
idea.

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

[Darrick J. Wong]

On Fri, Jul 03, 2026 at 06:03:26AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 02, 2026 at 12:58:14PM -0700, Celeste wrote:
> > should we then assume that this is fine to merge?
> 
> I still don't think that just papering over the misuse of the same type
> for on-disk and in-memory uses with different characteristics is a good
> idea.

I agree.

--D