On Thu, 8 Oct 2015, Martin Natano wrote:
> Filesystem implementations depend on vattr_null() to initialize the
> fields in struct vattr, which is true for all the fields except
> va_filerev. It therefore is not set to VNOVAL as expected by the file
> system, but contains whatever was there on the stack. This causes
> VOP_GETATTR() on cd9660 and msdosfs vnodes to yield garbage for
> va_filerev.
Thanks for the report. Fixing that made me think more closely about
vattr_null() and ended up totally rewriting it.
To back up...
Beware code that chains variable assignments to a potentially negative
value, such as:
foo = bar = baz = -1;
If foo, bar, and baz have different sizes, then the result may depend on
the order! To demonstrate:
#include <stdio.h>
int main(void)
{
unsigned long long llu;
unsigned u;
u = llu = -1; printf("u\t%#x\nllu\t%#llx\n", u, llu);
llu = u = -1; printf("u\t%#x\nllu\t%#llx\n", u, llu);
return 0;
}
That program outputs:
u 0xffffffff
llu 0xffffffffffffffff
u 0xffffffff
llu 0xffffffff
showing that the order of assignment matters. What's happening is that
the value of an assignment is of the type of the lvalue being assigned to,
so
llu = u = -1;
is the same as
llu = (unsigned)(u = -1);
Since (unsigned)-1 == 2^31-1 is already in the range of llu, there's no
expansion to 2^63-1. (I haven't verified, but assignment to an unsigned
type to a smaller, signed type may count as signed overflow and
technically be undefined behavior. If so and your compiler optimized on
that basis, beware!)
vattr_null() in the kernel was balanced on a knife's edge, switching among
*six* types in the third statement here:
/* XXX These next two used to be one line, but for a GCC bug. */
vap->va_size = VNOVAL;
vap->va_bytes = VNOVAL;
vap->va_mode = vap->va_nlink = vap->va_uid = vap->va_gid =
vap->va_fsid = vap->va_fileid =
vap->va_blocksize = vap->va_rdev =
vap->va_atime.tv_sec = vap->va_atime.tv_nsec =
vap->va_mtime.tv_sec = vap->va_mtime.tv_nsec =
vap->va_ctime.tv_sec = vap->va_ctime.tv_nsec =
vap->va_flags = vap->va_gen = VNOVAL;
That has all of int, u_int, long, u_long, quad_t, and u_quad_t, and only
works because it has a signed type on the right when assigning to a larger
type. It breaks if, for example, you swap va_nlink and va_fsid, or if you
swap the tv_sec and tv_nsec order, or if you put va_size or va_bytes on
the front of the third statement:
vap->va_size = vap->va_bytes = vap->va_mode = ...
I suspect the "GCC bug" comment was because the author (pre-rev 1.1)
didn't understand the C promotion rules.
So I've changed it to stop being fancy and just do the assignments one by
one. This also results in the assignments being done in memory order:
vap->va_type = VNON;
/*
* Don't get fancy: u_quad_t = u_int = VNOVAL leaves the u_quad_t
* with 2^31-1 instead of 2^64-1. Just write'm out and let
* the compiler do its job.
*/
vap->va_mode = VNOVAL;
vap->va_nlink = VNOVAL;
vap->va_uid = VNOVAL;
vap->va_gid = VNOVAL;
vap->va_fsid = VNOVAL;
...etc
For this sort of init/reinit function, we may update that to use a
static const variable with the desired values and just assign from that,
but that'll just be gravy compared to making to code robust to changes
to typedefs.
Philip Guenther
