NetBSD Problem Report #43963

From minoura@smtp.minoura.org  Tue Oct 12 14:10:48 2010
Return-Path: <minoura@smtp.minoura.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 3E8B763BAC4
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 12 Oct 2010 14:10:48 +0000 (UTC)
Message-Id: <20101012141045.C621C1AE69@smtp.minoura.org>
Date: Tue, 12 Oct 2010 23:10:45 +0900 (JST)
From: minoura@smtp.minoura.org
Reply-To: minoura@smtp.minoura.org
To: gnats-bugs@gnats.NetBSD.org
Subject: libsa cd9660.c pathname lookup problem
X-Send-Pr-Version: 3.95

>Number:         43963
>Category:       kern
>Synopsis:       libsa cd9660.c pathname lookup problem
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Oct 12 14:15:00 +0000 2010
>Closed-Date:    Tue Oct 19 02:40:50 +0000 2010
>Last-Modified:  Tue Oct 19 02:40:50 +0000 2010
>Originator:     Minoura Makoto
>Release:        NetBSD 5.0.2
>Organization:
>Environment:
System: NetBSD jumbo 5.0.2 NetBSD 5.0.2 (JUMBO) #18: Mon Mar 8 22:21:10 JST 2010 root@jumbo:/usr/obj/sys/arch/i386/compile/JUMBO i386
Architecture: i386
Machine: i386
>Description:
	cd9660_open() traverses the ISO9660 path table incorrectly,
	and fails to open files in subdirectories.
	I'm afraid the author misunderstood the order of the entries.
	Note it can open any files in the root directory successfully.
>How-To-Repeat:
	Put NetBSD/i386 CD-ROM and start the machine.
	At the > boot prompt, type boot i386/binary/kernel/netbsd-INSTALL_FLOPPY.gz etc.
>Fix:
Index: sys/lib/libsa/cd9660.c
===================================================================
RCS file: /cvsroot/src/sys/lib/libsa/cd9660.c,v
retrieving revision 1.23
diff -u -r1.23 cd9660.c
--- sys/lib/libsa/cd9660.c	24 Nov 2007 13:20:54 -0000	1.23
+++ sys/lib/libsa/cd9660.c	11 Oct 2010 11:41:02 -0000
@@ -205,9 +205,7 @@
 	while (*path) {
 		if ((char *)pp >= (char *)buf + psize)
 			break;
-		if (isonum_722(pp->parent) != parent)
-			break;
-		if (!pnmatch(path, pp)) {
+		if (!pnmatch(path, pp) || (isonum_722(pp->parent) != parent)) {
 			pp = (struct ptable_ent *)((char *)pp + PTSIZE(pp));
 			ent++;
 			continue;
@@ -215,12 +213,6 @@
 		path += isonum_711(pp->namlen) + 1;
 		parent = ent;
 		bno = isonum_732(pp->block) + isonum_711(pp->extlen);
-		while ((char *)pp < (char *)buf + psize) {
-			if (isonum_722(pp->parent) == parent)
-				break;
-			pp = (struct ptable_ent *)((char *)pp + PTSIZE(pp));
-			ent++;
-		}
 	}

 	/*

>Release-Note:

>Audit-Trail:
From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: gnats-bugs@NetBSD.org
Cc: minoura@smtp.minoura.org, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Tue, 12 Oct 2010 17:49:17 +0200

 Hi,

 > 	cd9660_open() traverses the ISO9660 path table incorrectly,
 > 	and fails to open files in subdirectories.
 > 	I'm afraid the author misunderstood the order of the entries.
 > 	Note it can open any files in the root directory successfully.

 No, that's not correct.  The author (yours truly) understood the order
 of the entries in the path table quite well.  It is mkisofs which is
 at fault here.

 You can check it yourself by looking at "6.9.1 Order of Path Table Records"
 in the ISO 9660 specification (to be found e.g. at
 http://www.ecma-international.org/publications/standards/Ecma-119.htm)
 There it says that the main criterion for the order is the level in the
 directory hierarchy, i.e. the list starts with all the entries at level
 1 (by definition only the root directory), then all the entries at level
 2 (all the entries in the root directory), then all entries at level 3
 and so on. The second criterion then specifies that the entries at some
 level are to be sorted according to the directory number of their parent
 directory.

 So the code is totally correct in terminating the search loop when finding
 a directory entry with a different parent directory identifier number.

 Now I'm (more or less) indifferent to lifting the insistance on correctly
 written 9660 filesystems (there is already another workaround in the kernel
 9660 implementation for incorrectly sorted directory entries in the main
 directory tree), however, in that case you should probably restart the
 search at the top of the path table after identifying a path component,
 in order to be even more tolerant to incorrectly formatted filesystems.

 Fixing mkisofs would be my preferred solution here (as the code in question
 is only used during boot, we don't have to support reading CDs from
 different sources anyway.)

 Ciao,
 Wolfgang
 -- 
 Wolfgang@Solfrank.net                          Wolfgang Solfrank

From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org, minoura@smtp.minoura.org, 
 kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org, 
 netbsd-bugs@NetBSD.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Tue, 12 Oct 2010 18:52:27 +0200

 >> Fixing mkisofs would be my preferred solution here
 >
 > Nowadays we use our own makefs(8) to create isos so it also has that bug?

 Actually, no, we don't yet AFAICT.  I've got no idea, why not.

 However, looking into the source of makefs, it seems that that does get it
 right (despite a comment before cd9660_generate_path_table, which pretends
 that the routine "could be done recursively", which would however result in
 a different incorrect order of the entries, at least if the recursion is
 done naively)

 Ciao,
 Wolfgang
 -- 
 Wolfgang@Solfrank.net                          Wolfgang Solfrank

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: Wolfgang@Solfrank.net
Cc: gnats-bugs@NetBSD.org, minoura@smtp.minoura.org,
        kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 02:03:05 +0900

 > >> Fixing mkisofs would be my preferred solution here
 > >
 > > Nowadays we use our own makefs(8) to create isos so it also has that bug?
 > 
 > Actually, no, we don't yet AFAICT.  I've got no idea, why not.

 What makes you think so?

 src/distrib/cdrom is obsolete and we have been using
 build.sh iso-image which uses src/distrib/common/Makefile.bootcd.
 (except mac68k and macppc, which require HFS/ISO hybrid)
 ---
 Izumi Tsutsui

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: Wolfgang@Solfrank.net
Cc: gnats-bugs@NetBSD.org, minoura@smtp.minoura.org,
        kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 01:06:35 +0900

 > Fixing mkisofs would be my preferred solution here

 Nowadays we use our own makefs(8) to create isos so it also has that bug?
 ---
 Izumi Tsutsui

From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: gnats-bugs@NetBSD.org
Cc: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
 minoura@smtp.minoura.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 01:38:36 +0200

 Hi,

 >   >  >  Nowadays we use our own makefs(8) to create isos so it also has that bug?
 >   >
 >   >  Actually, no, we don't yet AFAICT.  I've got no idea, why not.
 >
 >   What makes you think so?

 AFAICT "build.sh iso-image" does a "make iso-image" in the src-directory,
 which in turn does as its last action (apart of chit-chat) a "make iso-image"
 in the etc subdirectory of src, which only knows about mkisofs, nothing about
 makefs.

 Apart from that, as previously stated, makefs would get the order of path
 table entries correct (if my analysis of the code in there isn't
 completely wrong.)

 Ciao,
 Wolfgang
 -- 
 Wolfgang@Solfrank.net                          Wolfgang Solfrank

From: Martin Husemann <martin@duskware.de>
To: Wolfgang Solfrank <Wolfgang@Solfrank.net>
Cc: gnats-bugs@NetBSD.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 10:33:09 +0200

 It does "make iso_image" in src/distrib.

 Martin

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: Wolfgang@Solfrank.net
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org, minoura@smtp.minoura.org,
        tsutsui@ceres.dti.ne.jp
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 18:59:54 +0900

 > AFAICT "build.sh iso-image" does a "make iso-image" in the src-directory,

 Probably you are looking at leftover lines (for macs) in src/etc/Makefile:
 http://mail-index.NetBSD.org/source-changes/2007/03/06/0056.html
 http://mail-index.NetBSD.org/source-changes/2007/03/06/0059.html
 http://mail-index.NetBSD.org/source-changes/2007/03/06/0060.html
 (though I don't know if submitter used mkisofs or makefs)

 Anyway libsa should still work around buggy but major implementation
 like mkisofs(8) even if the author understood iso9660 spec correctly
 and it was implemented strictly as per the right spec.

 I wonder if the attached patch in this PR is "correct" in that scope,
 or we can have better workaround than it.

 ---
 Izumi Tsutsui

From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: gnats-bugs@NetBSD.org
Cc: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
 minoura@smtp.minoura.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Wed, 13 Oct 2010 15:50:08 +0200

 This is a multi-part message in MIME format.
 --------------010805080108070801080104
 Content-Type: text/plain; charset=ISO-8859-1; format=flowed
 Content-Transfer-Encoding: 7bit

 Hi again,

 so I looked closer at the problem.  Turns out that it's got nothing
 to do with the order of the path table entries, which is generated
 correctly by both mkisofs and makefs.

 The problem is that the code doesn't handle the very first entry in
 the path table, i.e. the entry for the root directory, correctly.
 The attached patch fixes this.  While here, it also fixes a minor
 bug, where the old code (would it have worked) would not allow multiple
 path separators betwween directory entries, i.e. it would not find
 i386///binary/kernel//netbsd_install_floppy.gz.

 As a minor nit to the original PR, the instructions in How-to-Repeat
 can't work anyway, as the 9660 boot code currently only supports
 original 9660 filenames (albeit allowing for lower/upper case conversion)
 which doesn't allow the '-' character in filenames.  You have to type

 boot i386/binary/kernel/netbsd_INSTALL_FLOPPY.gz

 to get the desired kernel (works, of course, only with the patch applied.)

 Ciao,
 Wolfgang
 -- 
 Wolfgang@Solfrank.net                          Wolfgang Solfrank

 --------------010805080108070801080104
 Content-Type: text/plain;
  name="diffs"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="diffs"

 Index: sys/lib/libsa/cd9660.c
 ===================================================================
 RCS file: /cvsroot/src/sys/lib/libsa/cd9660.c,v
 retrieving revision 1.25
 diff -u -r1.25 cd9660.c
 --- sys/lib/libsa/cd9660.c	22 Mar 2010 16:57:54 -0000	1.25
 +++ sys/lib/libsa/cd9660.c	13 Oct 2010 13:24:42 -0000
 @@ -190,19 +190,25 @@
  		goto out;
  	}

 -	parent = 1;
  	pp = (struct ptable_ent *)buf;
 -	ent = 1;
 +	/*
 +	 * The first entry is always the root directory.
 +	 * Since this has a special name in it, handle it separately.
 +	 */
 +	parent = 1;
  	bno = isonum_732(pp->block) + isonum_711(pp->extlen);
 +	pp = (struct ptable_ent *)((char *)pp + PTSIZE(pp));
 +	ent = 2;

  	rc = ENOENT;
 -	/*
 -	 * Remove extra separators
 -	 */
 -	while (*path == '/')
 -		path++;

  	while (*path) {
 +		/*
 +		 * Remove extra separators
 +		 */
 +		while (*path == '/')
 +			path++;
 +
  		if ((char *)pp >= (char *)buf + psize)
  			break;
  		if (isonum_722(pp->parent) != parent)
 @@ -212,7 +218,7 @@
  			ent++;
  			continue;
  		}
 -		path += isonum_711(pp->namlen) + 1;
 +		path += isonum_711(pp->namlen);
  		parent = ent;
  		bno = isonum_732(pp->block) + isonum_711(pp->extlen);
  		while ((char *)pp < (char *)buf + psize) {

 --------------010805080108070801080104--

From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, 
 netbsd-bugs@netbsd.org, minoura@smtp.minoura.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Fri, 15 Oct 2010 13:50:18 +0200

 This is a multi-part message in MIME format.
 --------------020504090603020104070009
 Content-Type: text/plain; charset=ISO-8859-1; format=flowed
 Content-Transfer-Encoding: 7bit

 Hi,

 ok, further investigation reveals that there is nothing wrong with the
 original code (except for the issue with multiple path separators.)
 The in-tree-code does exactly what it is designed to do.

 Note that the recipe from the How-to-Repeat in the PR does indeed fail,
 but that's the result of the '-' character in the filename, which is
 invalid in 9660 filenames, and not the result of some incorrectly
 parsed tables.  Note also, that the fix proposed in the PR doesn't do
 anything to change this.

 I'd be happy to change the code to handle the multiple '/' issue (see
 attached patch), but other than that, IMHO this PR should be closed.

 Ciao,
 Wolfgang
 -- 
 Wolfgang@Solfrank.net                          Wolfgang Solfrank

 --------------020504090603020104070009
 Content-Type: text/plain;
  name="diffs"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="diffs"

 Index: cd9660.c
 ===================================================================
 RCS file: /cvsroot/src/sys/lib/libsa/cd9660.c,v
 retrieving revision 1.25
 diff -u -r1.25 cd9660.c
 --- cd9660.c	22 Mar 2010 16:57:54 -0000	1.25
 +++ cd9660.c	15 Oct 2010 11:19:17 -0000
 @@ -196,13 +196,14 @@
  	bno = isonum_732(pp->block) + isonum_711(pp->extlen);

  	rc = ENOENT;
 -	/*
 -	 * Remove extra separators
 -	 */
 -	while (*path == '/')
 -		path++;

  	while (*path) {
 +		/*
 +		 * Remove extra separators
 +		 */
 +		while (*path == '/')
 +			path++;
 +
  		if ((char *)pp >= (char *)buf + psize)
  			break;
  		if (isonum_722(pp->parent) != parent)

 --------------020504090603020104070009--

From: minoura@minoura.org
To: Wolfgang Solfrank <Wolfgang@Solfrank.net>
Cc: gnats-bugs@NetBSD.org,  kern-bug-people@netbsd.org,  gnats-admin@netbsd.org,  netbsd-bugs@netbsd.org
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Sun, 17 Oct 2010 21:41:29 +0900

 Sorry for late reply; I was virtually AFK due to sudden
 illness of my family.  I still do not have time to look at
 the code but thank you for fixing the problem.

 -- 
 Minoura Makoto

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43963: libsa cd9660.c pathname lookup problem
Date: Mon, 18 Oct 2010 06:33:25 +0000

 On Fri, Oct 15, 2010 at 11:55:02AM +0000, Wolfgang Solfrank wrote:
  >  I'd be happy to change the code to handle the multiple '/' issue (see
  >  attached patch), but other than that, IMHO this PR should be closed.

 Can you commit that?

 -- 
 David A. Holland
 dholland@netbsd.org

From: "Wolfgang Solfrank" <ws@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/43963 CVS commit: src/sys/lib/libsa
Date: Mon, 18 Oct 2010 11:08:27 +0000

 Module Name:	src
 Committed By:	ws
 Date:		Mon Oct 18 11:08:26 UTC 2010

 Modified Files:
 	src/sys/lib/libsa: cd9660.c

 Log Message:
 Allow multiple path separators between/after directory names.
 Problem found while analyzing PR kern/43963.


 To generate a diff of this commit:
 cvs rdiff -u -r1.25 -r1.26 src/sys/lib/libsa/cd9660.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->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Tue, 19 Oct 2010 02:40:50 +0000
State-Changed-Why:
Fix for the path separator issue committed; the rest seems to be inherent
limitations of cd9660 volumes.


>Unformatted:
 	and probably -current.

Home	PR Database Search