Re: HEADS-UP new statfs structure

On Tue, 18 Nov 2003, Rudolf Cejka wrote:

> 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.)

Hmm, there were 2 bugs here:
- MFSNAMELEN was confused with MNAMELEN in some places.  This gives
  unterminated strings as well as excessively truncated strings.
- there were off-by-1 errors which would have given unterminated
  strings even without the previous bug.

> --- 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));

MFSNAMELEN didn't change, so there is currently only a logical problem
here.  The -1 term could be moved outside of the MIN().  It works in
either place and would save duplicating the terminating NUL in the
unlikely event that the new name length becomes smaller than the old
one.  I'm not sure which is clearest.

>  	bcopy(nsp->f_mntonname, osp->f_mntonname,
> -	    MIN(MFSNAMELEN, OMNAMELEN));
> +	    MIN(MNAMELEN, OMNAMELEN - 1));

Similarly, plus the larger bug.  MNAMELEN increased from
(88 - 2 * sizeof(long)) to 88, so if it were used without the -1 in
the above, then mount point name lengths longer than the old value
would have been unterminated instead of truncated.

>  	bcopy(nsp->f_mntfromname, osp->f_mntfromname,
> -	    MIN(MFSNAMELEN, OMNAMELEN));
> +	    MIN(MNAMELEN, OMNAMELEN - 1));

Similarly.

>  	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 */
>

MFSNAMELEN hasn't changed, so this part is cosmetic.  But don't we now need
to clone all of this compatibility cruft for the new statfs()?  Native
32-bit systems have both.  Then MFSNAMELEN for this version should probably
be spelled OMFSNAMELEN.

>  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);

Yikes.  All copied out structs that might have holes (i.e., all structs
unless you want to examine them in binary for every combination of
arch/compiler/etc) need to be bzero()ed like this, but there are no
bzero()'s in files in this directory.

>  	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

This seems to be correct except possibly for the style (placement of -1
and fixing the indentation of the continuation lines so that it is not
bug-for-bug compatible with the rest of the file).

> ---
>
> * 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.
> ...

Yikes :-).

Bruce