NetBSD Problem Report #48815

From www@NetBSD.org  Sat May 17 14:47:57 2014
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id B1112A5853
	for <gnats-bugs@gnats.NetBSD.org>; Sat, 17 May 2014 14:47:57 +0000 (UTC)
Message-Id: <20140517144756.17532A650D@mollari.NetBSD.org>
Date: Sat, 17 May 2014 14:47:56 +0000 (UTC)
From: scdbackup@gmx.net
Reply-To: scdbackup@gmx.net
To: gnats-bugs@NetBSD.org
Subject: mount_cd9660 option -o "norrip,gens" is quite unusable
X-Send-Pr-Version: www-1.0

>Number:         48815
>Category:       kern
>Synopsis:       mount_cd9660 option -o "norrip,gens" is quite unusable
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat May 17 14:50:00 +0000 2014
>Closed-Date:    Mon Jun 09 03:58:19 +0000 2014
>Last-Modified:  Mon Jun 09 03:58:19 +0000 2014
>Originator:     Thomas Schmitt
>Release:        6.1.3
>Organization:
>Environment:
NetBSD netbsd 6.1.3 NetBSD 6.1.3 (GENERIC) i386
>Description:
The options "norrip,gens" of mount_cd9660(8) cause peculiar behavior
of several shell tools, among them ls with not-working option -l.

ISO 9660 filenames end by a semicolon and a version number.
E.g. "SMALL_FILE.;1".
Usually these suffixes are not shown and not taken into respect
when looking up file names in the directory tree.

If Rock Ridge with its POSIX compliant file names is disabled,
and if option "gens" (ISOFSMNT_GENS) is in effect, then the
non-POSIX names are shown with semicolon and version number.

But they still cannot be looked up by names with such suffix.

The cause is this code snippet from sys/fs/cd9660/cd9660_util.c
function isofncmp(). It runs after both compared names matched
up to the first semicolon.
Now the version numbers are to be compared.

        fn++;
        for (i = 0; fnlen-- != 0; i = i * 10 + *fn++ - '0') {
                if (*fn < '0' || *fn > '9') {
                        return -1;
                }
        }

It increments fn, obviously to skip the semicolon. But this has been
done already by the call to static int wget() which produced the
16 bit variable which beared the semicolon.
Further, the third statement in for() increments fn again, before
the loop body can compare it with the digit range of ASCII.

So this is probably off track with its memory access by two bytes,
before it returns -1.

>How-To-Repeat:
It happens with any ISO 9660 filesystem which bears data files
(directories are supposed to have no version suffix):

  netbsd# mount_cd9660 -o norrip,nojoliet,gens /dev/cd0a /mnt/iso

  netbsd# ls /mnt/iso
  my              small_file.;1

  netbsd# ls -l /mnt/iso
  ls: small_file.;1: No such file or directory
  total 4
  dr-xr-xr-x  1 root  wheel  2048 May  6 15:30 my

  netbsd# ls /mnt/iso/small*
  ls: /mnt/iso/small_file.;1: No such file or directory

  netbsd# cat /mnt/iso/'small_file.;1' >/dev/null
  cat: /mnt/iso/small_file.;1: No such file or directory

The state is not totally useless, though:

  netbsd# cat /mnt/iso/small_file. | wc
         1       1       6

After the fix, the mounted test ISO yields:

  netbsd# ls -l /mnt/iso
  total 4
  dr-xr-xr-x  1 root  wheel  2048 May  6 15:30 my
  -r-xr-xr-x  1 root  wheel     6 May  6 15:34 small_file.;1

>Fix:
I believe it should be something like

        for (i = 0; fnlen-- != 0; fn++) {
                if (*fn < '0' || *fn > '9')
                        return -1;
                i = i * 10 + *fn - '0';
        }

although i'm still somewhat unhappy with the independent activities
on fnlen and fn. But the loop count seems to be ok. So i don't fiddle
with the second statement in for().

Patch proposal follows.

>Release-Note:

>Audit-Trail:
From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48815
Date: Sat, 17 May 2014 16:51:01 +0200

 --- sys/fs/cd9660/cd9660_util.c.orig	2014-05-13 20:39:31.000000000 +0000
 +++ sys/fs/cd9660/cd9660_util.c	2014-05-17 14:24:29.000000000 +0000
 @@ -117,11 +117,10 @@ isofncmp(const u_char *fn, size_t fnlen,
  			case ';':
  				break;
  			}
 -			fn++;
 -			for (i = 0; fnlen-- != 0; i = i * 10 + *fn++ - '0') {
 -				if (*fn < '0' || *fn > '9') {
 +			for (i = 0; fnlen-- != 0; fn++) {
 +				if (*fn < '0' || *fn > '9')
  					return -1;
 -				}
 +				i = i * 10 + *fn - '0';
  			}
  			for (j = 0; isofn != isoend; j = j * 10 + ic - '0')
  				isofn += isochar(isofn, isoend,

From: Wolfgang Solfrank <Wolfgang@Solfrank.net>
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: kern/48815: mount_cd9660 option -o "norrip,gens" is quite unusable
Date: Sat, 17 May 2014 18:09:34 +0200

 Hi,

 while your are correct on this:

 > It increments fn, obviously to skip the semicolon. But this has been
 > done already by the call to static int wget() which produced the
 > 16 bit variable which beared the semicolon.

 I'm unsure on what you are talking about here:

 > Further, the third statement in for() increments fn again, before
 > the loop body can compare it with the digit range of ASCII.

 as your proposed fix:

 >          for (i = 0; fnlen-- != 0; fn++) {
 >                  if (*fn < '0' || *fn > '9')
 >                          return -1;
 >                  i = i * 10 + *fn - '0';
 >          }

 it totally equivalent to the original code:

  >          for (i = 0; fnlen-- != 0; i = i * 10 + *fn++ - '0') {
  >                  if (*fn < '0' || *fn > '9') {
  >                          return -1;
  >                  }
  >          }

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

From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48815
Date: Sat, 17 May 2014 19:04:41 +0200

 Hi,

 Wolfgang Solfrank:
 > your proposed fix:
 > [...]
 > it totally equivalent to the original code:

 You are right. So the memory access is only off by one byte
 and the patch proposal becomes simpler.


From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48815
Date: Sat, 17 May 2014 19:10:26 +0200

 --- sys/fs/cd9660/cd9660_util.c.orig	2014-05-13 20:39:31.000000000 +0000
 +++ sys/fs/cd9660/cd9660_util.c	2014-05-17 17:02:53.000000000 +0000
 @@ -117,7 +117,6 @@ isofncmp(const u_char *fn, size_t fnlen,
  			case ';':
  				break;
  			}
 -			fn++;
  			for (i = 0; fnlen-- != 0; i = i * 10 + *fn++ - '0') {
  				if (*fn < '0' || *fn > '9') {
  					return -1;

From: "Thomas Schmitt" <scdbackup@gmx.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/48815: mount_cd9660 option -o "norrip,gens" is quite unusable
Date: Sat, 24 May 2014 10:15:49 +0200

 Ping ?

 The current proposal [1] is really a simple one.
 The statement in question is surplus according to code analysis,
 and it improves the filesystem behavior when being tested.

 A rare case of i-cannot-imagine-any-regression.

 [1] Patch as of Date: Sat, 17 May 2014 19:10:26 +0200

State-Changed-From-To: open->pending-pullups
State-Changed-By: martin@NetBSD.org
State-Changed-When: Sun, 01 Jun 2014 11:08:33 +0000
State-Changed-Why:
Commited, waiting for [pullup-6 #1073]


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48815 CVS commit: src/sys/fs/cd9660
Date: Sun, 1 Jun 2014 11:01:18 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sun Jun  1 11:01:18 UTC 2014

 Modified Files:
 	src/sys/fs/cd9660: cd9660_util.c

 Log Message:
 PR kern/48815: do not skip ';' twice when comparing file versions.
 Patch from Thomas Schmitt.


 To generate a diff of this commit:
 cvs rdiff -u -r1.10 -r1.11 src/sys/fs/cd9660/cd9660_util.c

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

From: "SAITOH Masanobu" <msaitoh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/48815 CVS commit: [netbsd-6] src/sys/fs/cd9660
Date: Tue, 3 Jun 2014 15:21:17 +0000

 Module Name:	src
 Committed By:	msaitoh
 Date:		Tue Jun  3 15:21:17 UTC 2014

 Modified Files:
 	src/sys/fs/cd9660 [netbsd-6]: cd9660_util.c

 Log Message:
 Pull up following revision(s) (requested by martin in ticket #1073):
 	sys/fs/cd9660/cd9660_util.c: revision 1.11
 PR kern/48815: do not skip ';' twice when comparing file versions.
 Patch from Thomas Schmitt.


 To generate a diff of this commit:
 cvs rdiff -u -r1.10 -r1.10.8.1 src/sys/fs/cd9660/cd9660_util.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: msaitoh@NetBSD.org
State-Changed-When: Mon, 09 Jun 2014 03:58:19 +0000
State-Changed-Why:
Pulled up to netbsd-6.
Thanks.


>Unformatted:

Home	PR Database Search