NetBSD Problem Report #52993
From www@NetBSD.org Thu Feb 8 11:09:43 2018
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id 1E5927A18A
for <gnats-bugs@gnats.NetBSD.org>; Thu, 8 Feb 2018 11:09:43 +0000 (UTC)
Message-Id: <20180208110941.C38697A231@mollari.NetBSD.org>
Date: Thu, 8 Feb 2018 11:09:41 +0000 (UTC)
From: "Harold Gutch" <logix@foobar.franken.de>
Reply-To: "Harold Gutch" <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Subject: case sensitive HFS+ can return data from wrong file
X-Send-Pr-Version: www-1.0
>Number: 52993
>Category: kern
>Synopsis: case sensitive HFS+ can return data from wrong file
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: kern-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Thu Feb 08 11:10:00 +0000 2018
>Closed-Date: Tue May 02 09:23:01 +0000 2023
>Last-Modified: Tue May 02 09:23:01 +0000 2023
>Originator: Harold Gutch
>Release: NetBSD 7.1 (but the bug exists from 5.0 to HEAD)
>Organization:
>Environment:
NetBSD 7.1 NetBSD 7.1 (GENERIC.201703111743Z) amd64
>Description:
On a case sensitive HFS+ file system, when locating a file of a given name, file names are compared against it as memcmp(name1, name2, min(len(name1), len(name2))). Thus, if name1 starts with name2 (or vice versa) they are understood to be identical and as a result when accessing name1 we might end up reading from name2 instead.
The issue does not occur for case insensitive HFS+ file systems (the default).
>How-To-Repeat:
1) On Mac OS, create a case sensitive HFS+ file system, create two files one's name starts with the other's, and fill them with different data.
osx:tmp hg$ hdiutil create test.dmg -size 1m -fs HFS+X -volname "test"
...............................................................................
created: /Users/hg/tmp/test.dmg
osx:tmp hg$ hdiutil mount test.dmg
/dev/disk7 GUID_partition_scheme
/dev/disk7s1 Apple_HFS /Volumes/test
osx:tmp hg$ echo foo > /Volumes/test/test
osx:tmp hg$ echo bar > /Volumes/test/test123
osx:tmp hg$ cat /Volumes/test/test
foo
osx:tmp hg$ cat /Volumes/test/test123
bar
2) After umounting, copy the .dmg over to NetBSD. There mount and check the inode numbers and the contents of the files.
ichiban# vndconfig vnd0 test.dmg
ichiban# gpt show vnd0 | grep HFS
40 1968 1 GPT part - Apple HFS
ichiban# dkctl vnd0 addwedge test 40 1968 hfs
dk0 created successfully.
ichiban# mount -t hfs -o ro /dev/dk0 /mnt
ichiban# cat /mnt/test
foo
ichiban# cat /mnt/test123
foo
ichiban# ls -li /mnt/test123 /mnt/test
22 -rw-r--r-- 1 503 staff 4 Feb 6 14:47 /mnt/test
22 -rw-r--r-- 1 503 staff 4 Feb 6 14:47 /mnt/test123
3) Notice that on NetBSD both, the contents of test and test123, and also their inode number are identical.
>Fix:
[ NOTE: the actual fix is in the few lines after "/* FIXME: ...", the rest is merely a bit of syntax sugar without functional change that just avoids the repeated casts to (const hfs_catalog_key_t*) all over the function and the same then again in hfslib_compare_extent_keys(). ]
--- src/sys/fs/hfs/libhfs.c.orig 2015-06-21 13:40:25.000000000 +0000
+++ src/sys/fs/hfs/libhfs.c 2018-02-06 11:51:04.000000000 +0000
@@ -2384,37 +2384,42 @@
/* binary compare (i.e., not case folding) */
int
hfslib_compare_catalog_keys_bc (
- const void *a,
- const void *b)
+ const void *ap,
+ const void *bp)
{
- if (((const hfs_catalog_key_t*)a)->parent_cnid
- == ((const hfs_catalog_key_t*)b)->parent_cnid)
+ int c;
+ const hfs_catalog_key_t *a, *b;
+
+ a = (const hfs_catalog_key_t *) ap;
+ b = (const hfs_catalog_key_t *) bp;
+
+ if (a->parent_cnid == b->parent_cnid)
{
- if (((const hfs_catalog_key_t*)a)->name.length == 0 &&
- ((const hfs_catalog_key_t*)b)->name.length == 0)
+ if (a->name.length == 0 && b->name.length == 0)
return 0;
- if (((const hfs_catalog_key_t*)a)->name.length == 0)
+ if (a->name.length == 0)
return -1;
- if (((const hfs_catalog_key_t*)b)->name.length == 0)
+ if (b->name.length == 0)
return 1;
/* FIXME: This does a byte-per-byte comparison, whereas the HFS spec
* mandates a uint16_t chunk comparison. */
- return memcmp(((const hfs_catalog_key_t*)a)->name.unicode,
- ((const hfs_catalog_key_t*)b)->name.unicode,
- min(((const hfs_catalog_key_t*)a)->name.length,
- ((const hfs_catalog_key_t*)b)->name.length));
+ c = memcmp(a->name.unicode, b->name.unicode,
+ sizeof(unichar_t)*min(a->name.length, b->name.length));
+ if (c != 0)
+ return c;
+ else
+ return (a->name.length - b->name.length);
} else {
- return (((const hfs_catalog_key_t*)a)->parent_cnid -
- ((const hfs_catalog_key_t*)b)->parent_cnid);
+ return (a->parent_cnid - b->parent_cnid);
}
}
int
hfslib_compare_extent_keys (
- const void *a,
- const void *b)
+ const void *ap,
+ const void *bp)
{
/*
* Comparison order, in descending importance:
@@ -2422,27 +2427,25 @@
* CNID -> fork type -> start block
*/
- if (((const hfs_extent_key_t*)a)->file_cnid
- == ((const hfs_extent_key_t*)b)->file_cnid)
+ const hfs_extent_key_t *a, *b;
+ a = (const hfs_extent_key_t *) ap;
+ b = (const hfs_extent_key_t *) bp;
+
+ if (a->file_cnid == b->file_cnid)
{
- if (((const hfs_extent_key_t*)a)->fork_type
- == ((const hfs_extent_key_t*)b)->fork_type)
+ if (a->fork_type == b->fork_type)
{
- if (((const hfs_extent_key_t*)a)->start_block
- == ((const hfs_extent_key_t*)b)->start_block)
+ if (a->start_block == b->start_block)
{
return 0;
} else {
- return (((const hfs_extent_key_t*)a)->start_block -
- ((const hfs_extent_key_t*)b)->start_block);
+ return (a->start_block - b->start_block);
}
} else {
- return (((const hfs_extent_key_t*)a)->fork_type -
- ((const hfs_extent_key_t*)b)->fork_type);
+ return (a->fork_type - b->fork_type);
}
} else {
- return (((const hfs_extent_key_t*)a)->file_cnid -
- ((const hfs_extent_key_t*)b)->file_cnid);
+ return (a->file_cnid - b->file_cnid);
}
}
>Release-Note:
>Audit-Trail:
From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/52993: case sensitive HFS+ can return data from wrong file
Date: Fri, 9 Feb 2018 16:35:20 +0000
I've uploaded a test disk image created as instructed in step #1
https://www.netbsd.org/~sevan/kern52993-test.dmg
Sevan
Responsible-Changed-From-To: kern-bug-people->sevan
Responsible-Changed-By: sevan@NetBSD.org
Responsible-Changed-When: Sat, 17 Feb 2018 00:44:07 +0000
Responsible-Changed-Why:
take
From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Fri, 28 Dec 2018 04:12:03 +0000
With the supplied patch the issue is still not resolved.
Sevan
From: Harold Gutch <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Sun, 30 Dec 2018 17:08:19 +0100
On Fri, Dec 28, 2018 at 04:15:00AM +0000, Sevan Janiyan wrote:
> With the supplied patch the issue is still not resolved.
It works for me. I have been using it all this time on a Raspberry Pi 1
(albeit only under light load) with multiple case sensitive HFS+ file
systems.
This here is from a vanilla 8.0 amd64 installation in VirtualBox,
with your disk image:
localhost# mv /stand/amd64/8.0/modules/hfs/hfs.kmod /stand/amd64/8.0/modules/hfs/hfs_orig.kmod
localhost# mv /stand/amd64/8.0/modules/hfs /stand/amd64/8.0/modules/hfs_orig
localhost# cd /usr/src/sys/fs/hfs
localhost# patch -p4 < patch
localhost# cd /usr/src
localhost# ./build.sh modules
localhost# mv /usr/obj/destdir.amd64/stand/amd64/8.0/modules/hfs /stand/amd64/8.0/modules/
localhost# vndconfig vnd0 kern52993-test.dmg
localhost# modload hfs_orig
localhost# mount -t hfs -o ro /dev/dk0 /mnt
localhost# ls -li /mnt/test /mnt/test123
23 -rw-r--r-- 1 501 staff 4 Feb 9 2018 /mnt/test
23 -rw-r--r-- 1 501 staff 4 Feb 9 2018 /mnt/test123
localhost# cat /mnt/test /mnt/test123
foo
foo
localhost# umount /mnt
localhost# modunload hfs
[ although the module's file name/directory is "hfs_orig",
the module's (internal) name still is "hfs" ]
localhost# modload hfs
localhost# mount -t hfs -o ro /dev/dk0 /mnt
localhost# ls -li /mnt/test /mnt/test123
23 -rw-r--r-- 1 501 staff 4 Feb 9 2018 /mnt/test
24 -rw-r--r-- 1 501 staff 4 Feb 9 2018 /mnt/test123
localhost# cat /mnt/test /mnt/test123
foo
bar
localhost#
If you want I can redo this test with current, but as sys/fs/hfs is
virtually unchanged this shouldn't change anything. I am not sure
what else I can add right now.
Or do you have some other issue with the patched module?
Harold
From: Sevan Janiyan <sevan@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Sun, 30 Dec 2018 22:10:24 +0000
Hi Harold,
On 30/12/2018 16:10, Harold Gutch wrote:
> If you want I can redo this test with current, but as sys/fs/hfs is
> virtually unchanged this shouldn't change anything. I am not sure
> what else I can add right now.
>
> Or do you have some other issue with the patched module?
Apologies, I made a mistake in that I hadn't realised HFS support was
via a loadable module, hence I only built a new kernel. I have just
built a new set of modules & can see the issue is now resolved.
optislab# vnconfig vnd0 /home/me/kern52993-test.dmg
optislab# mount_hfs /dev/dk0 /mnt
optislab# cat /mnt/test
foo
optislab# cat /mnt/test123
bar
optislab# ls -li /mnt/
total 42
20 -rw-r--r-- 1 501 staff 8196 Feb 9 2018 .DS_Store
17 dr-xr-xr-t 1 root wheel 512 Feb 9 2018 .HFS+ Private Directory
Data?
18 drwx------ 1 501 staff 512 Feb 9 2018 .fseventsd
23 -rw-r--r-- 1 501 staff 4 Feb 9 2018 test
24 -rw-r--r-- 1 501 staff 4 Feb 9 2018 test123
Sevan
From: "Sevan Janiyan" <sevan@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52993 CVS commit: src/sys/fs/hfs
Date: Sun, 30 Dec 2018 22:40:00 +0000
Module Name: src
Committed By: sevan
Date: Sun Dec 30 22:40:00 UTC 2018
Modified Files:
src/sys/fs/hfs: libhfs.c
Log Message:
Fix support for case sensitive HFS.
Without this change, the wrong file is returned, if 2 file names contain a
subset of each other.
Code submitted in PR bin/52993 by Harold Gutch
To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sys/fs/hfs/libhfs.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->needs-pullups
State-Changed-By: sevan@NetBSD.org
State-Changed-When: Sun, 30 Dec 2018 23:02:19 +0000
State-Changed-Why:
Need an ATF test too.
(in progress)
From: Harold Gutch <logix@foobar.franken.de>
To: gnats-bugs@NetBSD.org
Cc:
Subject: Re: kern/52993 (case sensitive HFS+ can return data from wrong file)
Date: Mon, 31 Dec 2018 00:04:56 +0100
Hi Sevan,
On Sun, Dec 30, 2018 at 10:15:00PM +0000, Sevan Janiyan wrote:
> Apologies, I made a mistake in that I hadn't realised HFS support was
> via a loadable module, hence I only built a new kernel. I have just
> built a new set of modules & can see the issue is now resolved.
Ah, OK. I think I only tried this with hfs as a module, but not
statically in-kernel or via puffs. Not that it *should* make a
difference of course.
thanks,
Harold
Responsible-Changed-From-To: sevan->kern-bug-people
Responsible-Changed-By: wiz@NetBSD.org
Responsible-Changed-When: Mon, 13 Jul 2020 20:12:38 +0000
Responsible-Changed-Why:
Reset to role account
State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 31 Mar 2023 09:44:24 +0000
State-Changed-Why:
pullup-8 #1817 https://releng.netbsd.org/cgi-bin/req-8.cgi?show=1817
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52993 CVS commit: [netbsd-8] src/sys/fs/hfs
Date: Sat, 1 Apr 2023 16:34:04 +0000
Module Name: src
Committed By: martin
Date: Sat Apr 1 16:34:04 UTC 2023
Modified Files:
src/sys/fs/hfs [netbsd-8]: libhfs.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #1817):
sys/fs/hfs/libhfs.c: revision 1.15
Fix support for case sensitive HFS.
Without this change, the wrong file is returned, if 2 file names contain a
subset of each other.
Code submitted in PR bin/52993 by Harold Gutch
To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.14.10.1 src/sys/fs/hfs/libhfs.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: hgutch@NetBSD.org
State-Changed-When: Tue, 02 May 2023 09:23:01 +0000
State-Changed-Why:
Pulled up to netbsd-8 so this now is in all supported branches.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2023
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home