NetBSD Problem Report #58887

From www@netbsd.org  Mon Dec  9 18:53:07 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 9C54A1A923A
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  9 Dec 2024 18:53:07 +0000 (UTC)
Message-Id: <20241209185306.4B0E11A923B@mollari.NetBSD.org>
Date: Mon,  9 Dec 2024 18:53:06 +0000 (UTC)
From: jlduran@gmail.com
Reply-To: jlduran@gmail.com
To: gnats-bugs@NetBSD.org
Subject: mtree: Duplicate entries in an mtree file cause mtree to coredump
X-Send-Pr-Version: www-1.0

>Number:         58887
>Category:       bin
>Synopsis:       mtree: Duplicate entries in an mtree file cause mtree to coredump
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Dec 09 18:55:00 +0000 2024
>Last-Modified:  Wed Dec 17 15:15:01 +0000 2025
>Originator:     Jose Luis Duran
>Release:        trunk
>Organization:
FreeBSD
>Environment:
NetBSD netbsd10.home.arpa 10.0 NetBSD 10.0 (GENERIC) #0: Thu Mar 28 08:33:33 UTC 2024  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64

>Description:
A FreeBSD bug was reported[^1] that the following mtree specification file would 
cause mtree to produce a core dump:

# cat test.mtree
/set type=dir
.
    dup
    ..
    dup
        child_entry     type=file

[^1]: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=192839

>How-To-Repeat:
# cat test.mtree
/set type=dir
.
    dup
    ..
    dup
        child_entry     type=file

# mtree -dEU -f test.mtree
[1]   Segmentation fault (core dumped) mtree -dEU -f test.mtree

>Fix:
Not really a fix, what I have so far is the following workaround (just avoids the SEGFAULT):

--- usr.sbin/mtree/spec.c
+++ usr.sbin/mtree/spec.c
@@ -260,6 +260,8 @@ noparent:		mtree_err("no parent node");
 				 * (after encountering ".." entry);
 				 * add or replace
 				 */
+			if (last->parent == NULL)
+				goto noparent;
 			centry->parent = last->parent;
 			addchild(last->parent, centry);
 			last = centry;


>Audit-Trail:
From: Jose Luis Duran <jlduran@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/58887
Date: Mon, 15 Dec 2025 02:22:41 -0300

 The comment on the original bug report:
 https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=192839#c7
 Describes clearly what's going on.

 The original naive workaround should have been to just avoid the free():

 --- usr.sbin/mtree/spec.c
 +++ usr.sbin/mtree/spec.c
 @@ -537,7 +537,6 @@ replacenode(NODE *cur, NODE *new)
         REPLACESTR(tags);
         REPLACE(lineno);
         REPLACE(flags);
 -       free(new);
  }

  static void
 @@ -779,8 +778,7 @@ addchild(NODE *pathparent, NODE *centry)
                 /*
                  * We found a node with the same name above.  Call
                  * replacenode(), which will either exit with an error,
 -                * or replace the information in the samename node and
 -                * free the information in the centry node.
 +                * or replace the information in the samename node.
                  */
                 replacenode(samename, centry);
                 if (samename == replacepos) {

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 jlduran@gmail.com
Subject: Re: bin/58887
Date: Wed, 17 Dec 2025 09:27:05 -0500

 --Apple-Mail=_42EC9238-67A7-49BB-B8F3-2653272BA1D7
 Content-Type: multipart/mixed;
 	boundary="Apple-Mail=_2718C70A-4F70-4571-9B0A-968C2A9DBDF7"


 --Apple-Mail=_2718C70A-4F70-4571-9B0A-968C2A9DBDF7
 Content-Transfer-Encoding: quoted-printable
 Content-Type: text/plain;
 	charset=us-ascii

 The problem is that addchild(..., centry) may free centry and use =
 samename instead, and then the caller site does: last =3D centry; =
 pointing to free memory. The correct fix I think is to have addchild =
 return the proper NODE to be assigned as last, instead of leaking memory =
 and using the old node.


 --Apple-Mail=_2718C70A-4F70-4571-9B0A-968C2A9DBDF7
 Content-Disposition: attachment;
 	filename=spec.c.diff
 Content-Type: application/octet-stream;
 	name=spec.c.diff;
 	x-unix-mode=0664
 Content-Transfer-Encoding: 7bit

 ? mt
 ? spec.c.dif
 ? spec.c.diff
 Index: spec.c
 ===================================================================
 RCS file: /cvsroot/src/usr.sbin/mtree/spec.c,v
 retrieving revision 1.93
 diff -u -p -u -r1.93 spec.c
 --- spec.c	13 Dec 2025 18:50:58 -0000	1.93
 +++ spec.c	17 Dec 2025 14:23:35 -0000
 @@ -100,7 +100,7 @@ static	dev_t	parsedev(char *);
  static	void	replacenode(NODE *, NODE *);
  static	void	set(char *, NODE *);
  static	void	unset(char *, NODE *);
 -static	void	addchild(NODE *, NODE *);
 +static	NODE	*addchild(NODE *, NODE *);
  static	int	nodecmp(const NODE *, const NODE *);
  static	int	appendfield(FILE *, int, const char *, ...) __printflike(3, 4);

 @@ -239,8 +239,7 @@ noparent:		mtree_err("no parent node");
  				 * full path entry; add or replace
  				 */
  			centry->parent = pathparent;
 -			addchild(pathparent, centry);
 -			last = centry;
 +			last = addchild(pathparent, centry);
  		} else if (strcmp(centry->name, ".") == 0) {
  				/*
  				 * duplicate "." entry; always replace
 @@ -252,8 +251,7 @@ noparent:		mtree_err("no parent node");
  				 * add or replace
  				 */
  			centry->parent = last;
 -			addchild(last, centry);
 -			last = centry;
 +			last = addchild(last, centry);
  		} else {
  				/*
  				 * new relative child in parent dir
 @@ -261,8 +259,7 @@ noparent:		mtree_err("no parent node");
  				 * add or replace
  				 */
  			centry->parent = last->parent;
 -			addchild(last->parent, centry);
 -			last = centry;
 +			last = addchild(last->parent, centry);
  		}
  	}
  	return (root);
 @@ -721,7 +718,7 @@ unset(char *t, NODE *ip)
   *	a duplicate, insert it into the linked list referenced by
   *	pathparent->child.  Keep the list sorted if Sflag is set.
   */
 -static void
 +static NODE *
  addchild(NODE *pathparent, NODE *centry)
  {
  	NODE *samename;      /* node with the same name as centry */
 @@ -740,7 +737,7 @@ addchild(NODE *pathparent, NODE *centry)
  	if (cur == NULL) {
  		/* centry is pathparent's first and only child node so far */
  		pathparent->child = centry;
 -		return;
 +		return centry;
  	}

  	/*
 @@ -785,7 +782,7 @@ addchild(NODE *pathparent, NODE *centry)
  		replacenode(samename, centry);
  		if (samename == replacepos) {
  			/* The just-replaced node was in the correct position */
 -			return;
 +			return samename;
  		}
  		if (samename == insertpos || samename->prev == insertpos) {
  			/*
 @@ -793,7 +790,7 @@ addchild(NODE *pathparent, NODE *centry)
  			 * or just after the replaced node, but that would
  			 * be equivalent to just retaining the replaced node.
  			 */
 -			return;
 +			return samename;
  		}

  		/*
 @@ -833,7 +830,7 @@ addchild(NODE *pathparent, NODE *centry)
  		if (centry->next)
  			centry->next->prev = centry;
  	}
 -	return;
 +	return centry;
  }

  /*

 --Apple-Mail=_2718C70A-4F70-4571-9B0A-968C2A9DBDF7
 Content-Transfer-Encoding: 7bit
 Content-Type: text/plain;
 	charset=us-ascii



 christos
 --Apple-Mail=_2718C70A-4F70-4571-9B0A-968C2A9DBDF7--

 --Apple-Mail=_42EC9238-67A7-49BB-B8F3-2653272BA1D7
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
 	filename=signature.asc
 Content-Type: application/pgp-signature;
 	name=signature.asc
 Content-Description: Message signed with OpenPGP

 -----BEGIN PGP SIGNATURE-----
 Comment: GPGTools - http://gpgtools.org

 iF0EARECAB0WIQS+BJlbqPkO0MDBdsRxESqxbLM7OgUCaUK9uQAKCRBxESqxbLM7
 OvVrAJ4/mjaklfFdWw/ghF5vXUAFVrs4LACg1rr6RD6I9iaMQSYnDdImTLNzRoQ=
 =F63A
 -----END PGP SIGNATURE-----

 --Apple-Mail=_42EC9238-67A7-49BB-B8F3-2653272BA1D7--

From: Jose Luis Duran <jlduran@gmail.com>
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: bin/58887
Date: Wed, 17 Dec 2025 12:10:17 -0300

 On Wed, Dec 17, 2025 at 11:30=E2=80=AFAM Christos Zoulas via gnats
 <gnats-admin@netbsd.org> wrote:
 >

 >  The problem is that addchild(..., centry) may free centry and use =3D
 >  samename instead, and then the caller site does: last =3D3D centry; =3D
 >  pointing to free memory. The correct fix I think is to have addchild =3D
 >  return the proper NODE to be assigned as last, instead of leaking memory=
  =3D
 >  and using the old node.

 Amazing! Yes, it makes absolute sense. I have tested the patch and it
 indeed avoids the use after free.
 Thank you!

Home	PR Database Search