Subject: Re: kern/28804
To: None <kern-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Edgar =?iso-8859-1?B?RnXf?= <ef@math.uni-bonn.de>
List: netbsd-bugs
Date: 02/11/2007 16:45:02
The following reply was made to PR kern/28804; it has been noted by GNATS.

From: Edgar =?iso-8859-1?B?RnXf?= <ef@math.uni-bonn.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/28804
Date: Sun, 11 Feb 2007 17:41:55 +0100

 I ran into the same problem yesterday.
 
 It looks to me like someone over-optimized things when importing the FFSv2 changes by McKusick. The value of ino is used later in the code.
 
 After that, I got no more "bad inode number 63 to nextinode", but SIGSEVs.
 
 It may be preferable to actually read in the superblock before using its fields to size malloc()s.
 
 Could someone actually understanding the code (contrary to me) be so kind as to have a look at fbd's version witch seems to incorporate some funtional changes (mostly by McKusick)?
 
 -- don't account for the accounting files
 -- don't account for snapshots either
 -- some optimizations for softdeps I don't understand
 
 Maybe it's more consistent to use is_ufsv2 throughout rather than checking for the v2 magic in some places instead.
 
 I'm also not entirely convinced that err() and warn() are used properly in the code, but that's probably due to my mis-understanding.
 
 The following patch seems to correct the most serious problems (at least it runs with no errors an no core dumps on an empty FFSv2 filesystem):
 
 --- quotacheck.c.orig	2004-12-12 06:57:03.000000000 +0100
 +++ quotacheck.c	2007-02-11 16:32:55.000000000 +0100
 @@ -1,5 +1,3 @@
 -/*	$NetBSD: quotacheck.c,v 1.36 2004/12/12 05:57:03 christos Exp $	*/
 -
  /*
   * Copyright (c) 1980, 1990, 1993
   *	The Regents of the University of California.  All rights reserved.
 @@ -360,19 +358,9 @@
  	sync();
  	dev_bsize = 1;
  
 -	cgp = malloc(sblock.fs_cgsize);
 -	if (cgp == NULL) {
 -		warn("%s: can't allocate %d bytes of cg space", fsname,
 -		    sblock.fs_cgsize);
 -		if (pid != NULL)
 -			exit(1);
 -		return 1;
 -	}
 -
  	for (i = 0; ; i++) {
  		if (sblock_try[i] == -1) {
  			warnx("%s: superblock not found", fsname);
 -			free(cgp);
  			if (pid != NULL)
  				exit(1);
  			return 1;
 @@ -411,10 +399,20 @@
  		break;
  	}
  
 +	cgp = malloc(sblock.fs_cgsize);
 +	if (cgp == NULL) {
 +		warn("%s: can't allocate %d bytes of cg space", fsname,
 +		    sblock.fs_cgsize);
 +		if (pid != NULL)
 +			exit(1);
 +		return 1;
 +	}
 +
  	dev_bsize = sblock.fs_fsize / fsbtodb(&sblock, 1);
  	maxino = sblock.fs_ncg * sblock.fs_ipg;
 -	for (ino = 0, cg = 0; cg < sblock.fs_ncg; cg++) {
 -		setinodebuf(cg * sblock.fs_ipg);
 +	for (cg = 0; cg < sblock.fs_ncg; cg++) {
 +		ino = cg * sblock.fs_ipg;
 +		setinodebuf(ino);
  #ifdef HAVE_UFSv2
  		if (sblock.fs_magic == FS_UFS2_MAGIC) {
  			bread(fsbtodb(&sblock, cgtod(&sblock, cg)), (char *)cgp,