From owner-freebsd-current@FreeBSD.ORG Tue Nov 18 08:46:29 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id F024F16A4CE for ; Tue, 18 Nov 2003 08:46:29 -0800 (PST) Received: from kazi.fit.vutbr.cz (kazi.fit.vutbr.cz [147.229.8.12]) by mx1.FreeBSD.org (Postfix) with ESMTP id 29DFA43F75 for ; Tue, 18 Nov 2003 08:46:28 -0800 (PST) (envelope-from cejkar@fit.vutbr.cz) Received: from kazi.fit.vutbr.cz (localhost [127.0.0.1]) by kazi.fit.vutbr.cz (8.12.10/8.12.9) with ESMTP id hAIGkJUC095549 (version=TLSv1/SSLv3 cipher=EDH-RSA-DES-CBC3-SHA bits=168 verify=NO); Tue, 18 Nov 2003 17:46:19 +0100 (CET) Received: (from cejkar@localhost) by kazi.fit.vutbr.cz (8.12.10/8.12.5/Submit) id hAIGkION095547; Tue, 18 Nov 2003 17:46:18 +0100 (CET) X-Authentication-Warning: kazi.fit.vutbr.cz: cejkar set sender to cejkar@fit.vutbr.cz using -f Date: Tue, 18 Nov 2003 17:46:18 +0100 From: Rudolf Cejka To: Kirk McKusick Message-ID: <20031118164618.GA84637@fit.vutbr.cz> References: <200311142119.hAELJPaG011962@beastie.mckusick.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200311142119.hAELJPaG011962@beastie.mckusick.com> User-Agent: Mutt/1.4.1i X-Scanned-By: MIMEDefang 2.16 (www . roaringpenguin . com / mimedefang) cc: freebsd-current@freebsd.org Subject: Re: HEADS-UP new statfs structure X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Nov 2003 16:46:30 -0000 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 @@ -645,11 +645,11 @@ 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 @@ -75,6 +75,7 @@ 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 { @@ -92,7 +93,7 @@ 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 @@ -276,6 +276,7 @@ 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); @@ -290,14 +291,14 @@ 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 @@ -88,7 +88,7 @@ { #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)) @@ -107,12 +107,12 @@ 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 http://www.fit.vutbr.cz/~cejkar Brno University of Technology, Faculty of Information Technology Bozetechova 2, 612 66 Brno, Czech Republic