NetBSD Problem Report #44523

From campbell@mumble.net  Sun Feb  6 17:58:24 2011
Return-Path: <campbell@mumble.net>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id AAB9F63B873
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  6 Feb 2011 17:58:24 +0000 (UTC)
Message-Id: <20110206175823.07ABF98298@pluto.mumble.net>
Date: Sun,  6 Feb 2011 17:58:23 +0000 (UTC)
From: Taylor R Campbell <campbell+netbsd@mumble.net>
Reply-To: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@gnats.NetBSD.org
Subject: mount_hfs badly handles file names with slashes in them
X-Send-Pr-Version: 3.95

>Number:         44523
>Category:       kern
>Synopsis:       mount_hfs badly handles file names with slashes in them
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Feb 06 18:00:00 +0000 2011
>Closed-Date:    Sun Feb 20 00:31:16 +0000 2011
>Last-Modified:  Sun Feb 20 00:31:16 +0000 2011
>Originator:     Taylor R Campbell <campbell+netbsd@mumble.net>
>Release:        NetBSD 5.1_STABLE
>Organization:
>Environment:
System: NetBSD smalltalk.local 5.1_STABLE NetBSD 5.1_STABLE (RIADEBUG) #0: Tue Feb 1 20:28:45 UTC 2011 root@smalltalk.local:/home/riastradh/netbsd/5/obj/sys/arch/i386/compile/RIADEBUG i386
Architecture: i386
Machine: i386
>Description:

	The kernel's HFS support takes slashes in file names on disk at
	face value and presents them to the userland.  Instead it
	should translate them to colons like Darwin does.

>How-To-Repeat:

	In Finder.app on Mac OS X, create a file in an HFS with a slash
	in its name.  (Alternatively, using any of the BSD subsystem in
	Mac OS X, create a file with a colon in its name.)  Mount the
	file system in NetBSD with mount_hfs or rump_hfs.  Try to use
	the file.  Fail.

>Fix:

	Apply the following patch to hfs_vnops.c to translate between
	slashes and colons: colons are not allowed on disk in the HFS,
	because of the old Macintosh pathname syntax.  (There is some
	trailing whitespace nearby.  If you apply this patch, you may
	want to take the opportunity to fix that while you're at it.)

Index: hfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/hfs/hfs_vnops.c,v
retrieving revision 1.18
diff -c -r1.18 hfs_vnops.c
--- hfs_vnops.c	24 Jun 2010 13:03:09 -0000	1.18
+++ hfs_vnops.c	6 Feb 2011 17:48:03 -0000
@@ -399,6 +399,7 @@
 	} else {
 		hfs_callback_args cbargs;
 		uint8_t len;
+		int ni;

 		hfslib_init_cbargs(&cbargs);

@@ -407,6 +408,9 @@
 		unicn = malloc(cnp->cn_namelen*sizeof(unicn[0]), M_TEMP, M_WAITOK);
 		len = utf8_to_utf16(unicn, cnp->cn_namelen,
 				    cnp->cn_nameptr, cnp->cn_namelen, 0, NULL);
+		for (ni = 0; ni < len; ni += 1)
+			if ((unichar_t)'/' == unicn[ni])
+				unicn[ni] = (unichar_t)':';
 		/* XXX: check conversion errors? */
 		if (hfslib_make_catalog_key(VTOH(vdp)->h_rec.u.cnid, len, unicn,
 			&key) == 0) {
@@ -912,6 +916,7 @@

 /*printf("numchildren = %i\n", numchildren);*/
 	for (curchild = 0; curchild < numchildren && uio->uio_resid>0; curchild++) {
+		int ni;
 		namlen = utf16_to_utf8(curent.d_name, MAXNAMLEN, 
 				       childnames[curchild].unicode,
 				       childnames[curchild].length,
@@ -921,6 +926,10 @@
 			/* XXX: how to handle name too long? */
 			continue;
 		}
+		/* XXX: perhaps check for colons too */
+		for (ni = 0; ni < namlen; ni += 1)
+			if ('/' == curent.d_name[ni])
+				curent.d_name[ni] = ':';
 		curent.d_namlen = namlen;
 		curent.d_reclen = _DIRENT_SIZE(&curent);


>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44523 CVS commit: src/sys/fs/hfs
Date: Wed, 9 Feb 2011 20:49:52 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Thu Feb 10 01:49:52 UTC 2011

 Modified Files:
 	src/sys/fs/hfs: hfs_vnops.c

 Log Message:
 PR/44523: Taylor R Campbell: mount_hfs badly handles file names with slashes
 in them, encode them as colons. XXX: Should encode : as :: too?


 To generate a diff of this commit:
 cvs rdiff -u -r1.20 -r1.21 src/sys/fs/hfs/hfs_vnops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
	christos@netbsd.org
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Thu, 10 Feb 2011 03:16:17 +0000

    Date: Thu, 10 Feb 2011 01:50:05 +0000 (UTC)
    From: "Christos Zoulas" <christos@netbsd.org>

    PR/44523: Taylor R Campbell: mount_hfs badly handles file names with sla=
 shes
    in them, encode them as colons. XXX: Should encode : as :: too?

 No, I don't think that's worth the trouble: as far as I know, the
 on-disk format is not supposed to have colons in file names, whereas
 it is supposed to be able to have slashes in file names, because the
 old Macintosh pathname syntax used colons rather than slashes as
 separators.

    +		/* XXX: perhaps check for colons too, and encode them? */
    +		for (ni =3D 0; ni < len; ni++)
    +			if (unicn[ni] =3D=3D (unichar_t)'/')
    +				unicn[ni] =3D (unichar_t)':';

 Oops -- I got this backwards in the patch I submitted.  (I must have
 tweaked the patch after testing it and before sending it.)  This
 fragment in hfs_vop_lookup should replace colon by slash, not the
 other way around.

From: christos@zoulas.com (Christos Zoulas)
To: Taylor R Campbell <campbell+netbsd@mumble.net>, gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Wed, 9 Feb 2011 22:31:50 -0500

 On Feb 10,  3:16am, campbell+netbsd@mumble.net (Taylor R Campbell) wrote:
 -- Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs

 | Oops -- I got this backwards in the patch I submitted.  (I must have
 | tweaked the patch after testing it and before sending it.)  This
 | fragment in hfs_vop_lookup should replace colon by slash, not the
 | other way around.


 All fixed, thanks!

 christos

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, christos@zoulas.com (Christos Zoulas)
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Thu, 10 Feb 2011 03:41:46 +0000

    Date: Thu, 10 Feb 2011 03:35:02 +0000 (UTC)
    From: christos@zoulas.com (Christos Zoulas)

    On Feb 10,  3:16am, campbell+netbsd@mumble.net (Taylor R Campbell) wrote:
    -- Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs

    | Oops -- I got this backwards in the patch I submitted.  (I must have
    | tweaked the patch after testing it and before sending it.)  This
    | fragment in hfs_vop_lookup should replace colon by slash, not the
    | other way around.

    All fixed, thanks!

 Works now.  Thanks!

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Sat, 12 Feb 2011 20:41:54 +0000

 On Thu, Feb 10, 2011 at 04:50:05AM +0000, Taylor R Campbell wrote:
  >     All fixed, thanks!
  >  
  >  Works now.  Thanks!

 Here's a patch for -5 (it doesn't merge cleanly) - I don't have the
 materials to test this, so it would be helpful if someone else
 could...

 Index: hfs_vnops.c
 ===================================================================
 RCS file: /cvsroot/src/sys/fs/hfs/hfs_vnops.c,v
 retrieving revision 1.11
 diff -u -p -r1.11 hfs_vnops.c
 --- hfs_vnops.c	3 Sep 2008 22:57:46 -0000	1.11
 +++ hfs_vnops.c	12 Feb 2011 20:40:30 -0000
 @@ -398,7 +398,7 @@ hfs_vop_lookup(void *v)
  		*vpp = vdp;
  	} else {
  		hfs_callback_args cbargs;
 -		uint8_t len;
 +		uint8_t len, ni;

  		hfslib_init_cbargs(&cbargs);

 @@ -407,6 +407,9 @@ hfs_vop_lookup(void *v)
  		unicn = malloc(cnp->cn_namelen*sizeof(unicn[0]), M_TEMP, M_WAITOK);
  		len = utf8_to_utf16(unicn, cnp->cn_namelen,
  				    cnp->cn_nameptr, cnp->cn_namelen, 0, NULL);
 +		for (ni = 0; ni < len; ni++)
 +			if (unicn[ni] == (unichar_t)':')
 +				unicn[ni] = (unichar_t)'/';
  		/* XXX: check conversion errors? */
  		if (hfslib_make_catalog_key(VTOH(vdp)->h_rec.u.cnid, len, unicn,
  			&key) == 0) {
 @@ -861,7 +864,7 @@ struct vop_readdir_args /* {
  	off_t bufoff; /* current position in buffer relative to start of dirents */
  	uint32_t numchildren;
  	uint32_t curchild; /* index of child we're currently stuffing into dirent */
 -	size_t namlen;
 +	size_t namlen, ni;
  	int error;
  	int i; /* dummy variable */

 @@ -906,6 +909,9 @@ struct vop_readdir_args /* {
  			/* XXX: how to handle name too long? */
  			continue;
  		}
 +		for (ni = 0; ni < namlen; ni++)
 +			if (curent.d_name[ni] == '/')
 +				curent.d_name[ni] = ':';
  		curent.d_namlen = namlen;
  		curent.d_reclen = _DIRENT_SIZE(&curent);



 -- 
 David A. Holland
 dholland@netbsd.org

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, David Holland <dholland-bugs@netbsd.org>
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Sat, 12 Feb 2011 21:25:08 +0000

    Date: Sat, 12 Feb 2011 20:41:54 +0000
    From: David Holland <dholland-bugs@netbsd.org>

    Here's a patch for -5 (it doesn't merge cleanly) - I don't have the
    materials to test this, so it would be helpful if someone else
    could...

 Will test shortly, if I remember.  Here's some material with which to
 make an automatic test, if anyone is so inclined:

 <http://mumble.net/~campbell/tmp/colon.hfs>,

 generated on Mac OS X by incanting:

 % mkdir /tmp/hfstest
 % cd /tmp/hfstest
 % echo lose > foo:bar
 % hdiutil create -srcfolder . -fs 'Case-sensitive HFS+' -layout NONE -nospo=
 tlight -format UFBI /tmp/colon.hfs

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, David Holland <dholland-bugs@netbsd.org>
Subject: Re: PR/44523 CVS commit: src/sys/fs/hfs
Date: Sun, 13 Feb 2011 22:42:01 +0000

    Date: Sat, 12 Feb 2011 20:41:54 +0000
    From: David Holland <dholland-bugs@netbsd.org>

    On Thu, Feb 10, 2011 at 04:50:05AM +0000, Taylor R Campbell wrote:
     >     All fixed, thanks!
     >  
     >  Works now.  Thanks!

    Here's a patch for -5 (it doesn't merge cleanly) - I don't have the
    materials to test this, so it would be helpful if someone else
    could...

 Just checked: works for me.

State-Changed-From-To: open->pending-pullups
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 14 Feb 2011 01:11:33 +0000
State-Changed-Why:
pullup-5 #1554. Before closing this we should also try to get a test
written.


From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44523 CVS commit: [netbsd-5] src/sys/fs/hfs
Date: Wed, 16 Feb 2011 21:20:04 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Wed Feb 16 21:20:04 UTC 2011

 Modified Files:
 	src/sys/fs/hfs [netbsd-5]: hfs_vnops.c

 Log Message:
 Pull up following revision(s) (requested by dholland in ticket #1554):
 	sys/fs/hfs/hfs_vnops.c: revision 1.21, 1.22 via patch
 PR/44523: Taylor R Campbell: mount_hfs badly handles file names with slashes
 in them, encode them as colons. XXX: Should encode : as :: too?
 remove comments about needing to encode : since the on disk format does
 not allow them. Also fix reversed encoding in lookup. From Taylor R Campbell.


 To generate a diff of this commit:
 cvs rdiff -u -r1.11 -r1.11.4.1 src/sys/fs/hfs/hfs_vnops.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
	gnats-admin@netbsd.org, dholland@NetBSD.org
Subject: Re: kern/44523 (mount_hfs badly handles file names with slashes in them)
Date: Thu, 17 Feb 2011 18:24:38 +0000

    Date: Mon, 14 Feb 2011 01:11:34 +0000 (UTC)
    From: dholland@NetBSD.org

    Before closing this we should also try to get a test written.

 I spent a few minutes today trying to figure out how to build and run
 tests.  I eventually gave up, but here's a trivial test I wrote (and
 haven't tested) for a src/tests/fs/hfs/t_colonslash.c or something, to
 be used with the HFS+ image I made:

 /*	$NetBSD$	*/

 #include <sys/types.h>
 #include <sys/mount.h>

 #include <atf-c.h>
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <string.h>

 #include <rump/rump.h>
 #include <rump/rump_syscalls.h>

 #include <fs/hfs/hfs.h>

 #include "../../h_macros.h"

 ATF_TC(colonslash);
 ATF_TC_HEAD(colonslash, tc)
 {
 	atf_tc_set_md_var(tc, "descr", "HFS+ colons/slashes (PR kern/44523)");
 	atf_tc_set_md_var(tc, "timeout", "2");
 }

 #define IMGNAME "colon.hfs"
 #define FAKEBLK "/dev/blk"
 #define FUNNY_FILENAME "foo:bar"
 ATF_TC_BODY(colonslash, tc)
 {
 	struct hfs_args args;
 	int dirfd, fd;
 	char buf[DIRBLKSIZ];
 	struct dirent *dirent;
 	int offset, nbytes;
 	bool ok = false;

 	memset(&args, 0, sizeof args);
 	args.fspec = __UNCONST(FAKEBLK);
 	rump_init();

 	if (rump_sys_mkdir("/mp", 0777) == -1)
 		atf_tc_fail_errno("cannot create mountpoint");

 	rump_pub_etfs_register(FAKEBLK, IMGNAME, RUMP_ETFS_BLK);
 	if (rump_sys_mount(MOUNT_HFS, "/mp", 0, &args, sizeof args) == -1)
 		atf_tc_fail_errno("rump_sys_mount failed");

 	dirfd = rump_sys_open("/mp", O_RDONLY);
 	if (dirfd == -1)
 		atf_tc_fail_errno("rump_sys_open failed");

 	nbytes = rump_sys_getdents(dirfd, buf, sizeof buf);
 	if (nbytes == -1)
 		atf_tc_fail_errno("rump_sys_getdents failed");

 	for (offset = 0; offset < nbytes; offset += dirent->d_reclen) {
 		dirent = (struct dirent *)(buf + offset);
 		if (!memchr(dirent->d_name, 0, dirent->d_namlen))
 			atf_tc_fail("dirent not null-terminated");
 		if (strchr(dirent->d_name, '/'))
 			atf_tc_fail("dirent with slash: %s", dirent->d_name);
 		if (0 == strcmp(FUNNY_FILENAME, dirent->d_name))
 			ok = true;
 	}

 	if (!ok)
 		atf_tc_fail("no dirent for file: %s", FUNNY_FILENAME);

 	if (rump_sys_close(dirfd) == -1)
 		atf_tc_fail_errno("rump_sys_close failed");

 	fd = rump_sys_open("/mp/" FUNNY_FILENAME, O_RDONLY);
 	if (fd == -1)
 		atf_tc_fail_errno("rump_sys_open failed");

 	if (rump_sys_close(fd) == -1)
 		atf_tc_fail_errno("rump_sys_close failed");

 	if (rump_sys_unmount("/mp", 0) == -1)
 		atf_tc_fail_errno("rump_sys_unmount failed");
 }

 ATF_TP_ADD_TCS(tp)
 {
 	ATF_TP_ADD_TC(tp, colonslash);
 	return 0;
 }

From: "Antti Kantee" <pooka@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44523 CVS commit: src/tests/fs
Date: Fri, 18 Feb 2011 13:07:54 +0000

 Module Name:	src
 Committed By:	pooka
 Date:		Fri Feb 18 13:07:54 UTC 2011

 Modified Files:
 	src/tests/fs: Makefile
 Added Files:
 	src/tests/fs/hfs: Makefile colon.hfs.bz2.uue t_pathconvert.c

 Log Message:
 Add test case for /->: conversion from PR kern/44523 by
 Taylor R Campbell.

 I adjusted the test to uudecode + bunzip2 the supplied image, and
 removed the "null-finder" from the dirent code, since it had an
 off-by-one which made the test fail.


 To generate a diff of this commit:
 cvs rdiff -u -r1.19 -r1.20 src/tests/fs/Makefile
 cvs rdiff -u -r0 -r1.1 src/tests/fs/hfs/Makefile \
     src/tests/fs/hfs/colon.hfs.bz2.uue src/tests/fs/hfs/t_pathconvert.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 20 Feb 2011 00:31:16 +0000
State-Changed-Why:
Fixed, pulled up, and pooka took care of the test case.
thanks!


>Unformatted:

Home	PR Database Search