Re: HEADS-UP new statfs structure

Kirk McKusick wrote (2003/11/14):
> This is why we make this change now so that it will be in place
> for the masses when 5.2 is released :-)

Hello, and is it possible to review some my (one's from masses :o)
questions/suggestions?

* cvtstatfs() for freebsd4_* compat syscalls does not copy text fields
  correctly, so old binaries with new kernel know just about first
  16 characters from mount points - what do you think about the
  following patch? (Or maybe with even safer sizeof() - but I did not
  test it.)

--- sys/kern/vfs_syscalls.c.orig	Sun Nov 16 11:12:09 2003
+++ sys/kern/vfs_syscalls.c	Sun Nov 16 11:56:07 2003
_at__at_ -645,11 +645,11 _at__at_
 	osp->f_syncreads = MIN(nsp->f_syncreads, LONG_MAX);
 	osp->f_asyncreads = MIN(nsp->f_asyncreads, LONG_MAX);
 	bcopy(nsp->f_fstypename, osp->f_fstypename,
-	    MIN(MFSNAMELEN, OMNAMELEN));
+	    MIN(MFSNAMELEN, OMFSNAMELEN - 1));
 	bcopy(nsp->f_mntonname, osp->f_mntonname,
-	    MIN(MFSNAMELEN, OMNAMELEN));
+	    MIN(MNAMELEN, OMNAMELEN - 1));
 	bcopy(nsp->f_mntfromname, osp->f_mntfromname,
-	    MIN(MFSNAMELEN, OMNAMELEN));
+	    MIN(MNAMELEN, OMNAMELEN - 1));
 	if (suser(td)) {
 		osp->f_fsid.val[0] = osp->f_fsid.val[1] = 0;
 	} else {
---

* sys/compat/freebsd32/freebsd32_misc.c: If you look into copy_statfs(),
  you copy 88-byte strings into just 80-byte strings. Fortunately it seems
  that there are just overwritten spare fields and f_syncreads/f_asyncreads
  before they are set to the correct value. What about these patches, which
  furthermore are resistant to possible MFSNAMELEN change in the future?
  [I'm sorry, these patches are untested]

--- sys/compat/freebsd32/freebsd32.h.orig	Tue Nov 18 16:58:28 2003
+++ sys/compat/freebsd32/freebsd32.h	Tue Nov 18 16:59:36 2003
_at__at_ -75,6 +75,7 _at__at_
 	int32_t	ru_nivcsw;
 };
 
+#define FREEBSD32_MFSNAMELEN      16 /* length of type name including null */
 #define FREEBSD32_MNAMELEN        (88 - 2 * sizeof(int32_t)) /* size of on/from name bufs */
 
 struct statfs32 {
_at__at_ -92,7 +93,7 _at__at_
 	int32_t	f_flags;
 	int32_t	f_syncwrites;
 	int32_t	f_asyncwrites;
-	char	f_fstypename[MFSNAMELEN];
+	char	f_fstypename[FREEBSD32_MFSNAMELEN];
 	char	f_mntonname[FREEBSD32_MNAMELEN];
 	int32_t	f_syncreads;
 	int32_t	f_asyncreads;
--- sys/compat/freebsd32/freebsd32_misc.c.orig	Tue Nov 18 16:59:49 2003
+++ sys/compat/freebsd32/freebsd32_misc.c	Tue Nov 18 17:03:31 2003
_at__at_ -276,6 +276,7 _at__at_
 static void
 copy_statfs(struct statfs *in, struct statfs32 *out)
 {
+	bzero(out, sizeof *out);
 	CP(*in, *out, f_bsize);
 	CP(*in, *out, f_iosize);
 	CP(*in, *out, f_blocks);
_at__at_ -290,14 +291,14 _at__at_
 	CP(*in, *out, f_flags);
 	CP(*in, *out, f_syncwrites);
 	CP(*in, *out, f_asyncwrites);
-	bcopy(in->f_fstypename,
-	      out->f_fstypename, MFSNAMELEN);
-	bcopy(in->f_mntonname,
-	      out->f_mntonname, MNAMELEN);
+	bcopy(in->f_fstypename, out->f_fstypename,
+	    MIN(MFSNAMELEN, FREEBSD32_MFSNAMELEN - 1));
+	bcopy(in->f_mntonname, out->f_mntonname,
+	    MIN(MNAMELEN, FREEBSD32_MNAMELEN - 1));
 	CP(*in, *out, f_syncreads);
 	CP(*in, *out, f_asyncreads);
-	bcopy(in->f_mntfromname,
-	      out->f_mntfromname, MNAMELEN);
+	bcopy(in->f_mntfromname, out->f_mntfromname,
+	    MIN(MNAMELEN, FREEBSD32_MNAMELEN - 1));
 }
 
 int
---
  
* sys/ia64/ia32/ia32.h: It seems to me that statfs32 has similar problems
  as freebsd32.h - however in this case real and bigger, because statfs32
  is now a combination of old and new statfs: old 32bit fields with new
  string lengths, so sizeof(statfs32) is changed from 256 to 272. Is this
  really correct? If I understand it correctly, it breaks binary
  compatibility for old ia32 binaries. I think that MFSNAMELEN/MNAMELEN
  should be renamed to OMFSNAMELEN/OMNAMELEN, or ia32.h should define own
  IA32_MFSNAMELEN/IA32_MNAMELEN, similarly to freebsd32.h - which is more
  secure with respect to further updates in the future.

* sys/ia64/ia32/ia32_misc.c: If ia32.h is changed/fixed, copy_statfs()
  has the same problems, as is in freebsd32_misc.c.

* sys/alpha/osf1/osf1_mount.c: And just small trash at the end, because
  it is in #ifdef notanymore ... #endif ;o) (however it means, that osf1
  statfs calls are completely broken?), but why bsd2osf_statfs() has
  3 x max()? What about following patch?

--- sys/alpha/osf1/osf1_mount.c.orig	Tue Nov 18 17:40:08 2003
+++ sys/alpha/osf1/osf1_mount.c	Tue Nov 18 17:40:52 2003
_at__at_ -88,7 +88,7 _at__at_
 {
 
 #ifdef notanymore	
-bzero(osfs, sizeof (struct osf1_statfs));
+	bzero(osfs, sizeof (struct osf1_statfs));
 	if (!strncmp(fsnames[MOUNT_UFS], bsfs->f_fstypename, MFSNAMELEN))
 		osfs->f_type = OSF1_MOUNT_UFS;
 	else if (!strncmp(fsnames[MOUNT_NFS], bsfs->f_fstypename, MFSNAMELEN))
_at__at_ -107,12 +107,12 _at__at_
 	osfs->f_files = bsfs->f_files;
 	osfs->f_ffree = bsfs->f_ffree;
 	bcopy(&bsfs->f_fsid, &osfs->f_fsid,
-	    max(sizeof bsfs->f_fsid, sizeof osfs->f_fsid));
+	    min(sizeof bsfs->f_fsid, sizeof osfs->f_fsid));
 	/* osfs->f_spare zeroed above */
 	bcopy(bsfs->f_mntonname, osfs->f_mntonname,
-	    max(sizeof bsfs->f_mntonname, sizeof osfs->f_mntonname));
+	    min(sizeof bsfs->f_mntonname, sizeof osfs->f_mntonname - 1));
 	bcopy(bsfs->f_mntfromname, osfs->f_mntfromname,
-	    max(sizeof bsfs->f_mntfromname, sizeof osfs->f_mntfromname));
+	    min(sizeof bsfs->f_mntfromname, sizeof osfs->f_mntfromname - 1));
 	/* XXX osfs->f_xxx should be filled in... */
 #endif
 }

Best regards.

-- 
Rudolf Cejka <cejkar at fit.vutbr.cz> http://www.fit.vutbr.cz/~cejkar
Brno University of Technology, Faculty of Information Technology
Bozetechova 2, 612 66  Brno, Czech Republic