Coda File System

Re: Re (and a patch) : Coda git repository available

From: Jan Harkes <jaharkes_at_cs.cmu.edu>
Date: Fri, 22 Apr 2016 11:55:46 -0400
On Fri, Apr 22, 2016 at 09:32:17AM +0200, u-myfx_at_aetey.se wrote:
>  coda-src/vtools/cfs.cc |   27 +++++++++++++++++++--------
>  1 files changed, 19 insertions(+), 8 deletions(-)
> 
> --- a/coda-src/vtools/cfs.cc
> +++ b/coda-src/vtools/cfs.cc
> @@ -1826,8 +1826,8 @@ static void ListVolume(int argc, char *argv[], int opslot)
>      VolumeStatus *vs;
>      char *volname, *omsg, *motd;
>      VolumeStateType conn_state;
> -    int conflict, cml_count;
> -    unsigned int age, time;
> +    int32_t conflict, cml_count;
> +    uint32_t age, time;
>      uint64_t cml_bytes;
>      char *ptr;
>      int local_only = 0;
> @@ -1859,25 +1859,36 @@ static void ListVolume(int argc, char *argv[], int opslot)
>  	   cml_count, offlinemsg, motd, age, time, cml_bytes) */
>  	ptr = piobuf;		/* invariant: ptr always point to next obj
>  				   to be read */
> +/* we hope that piobuf is sufficiently well aligned */

It should because as far as I remember to make it easier on the kernel
it is always aligned on a page boundary and always smaller than the size
of a memory page.

>  	vs = (VolumeStatus *)ptr;
>  	ptr += sizeof(VolumeStatus);
>  	volname = ptr; ptr += strlen(volname)+1;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here is the real culprit, if those strings were padded to the next
32-bit boundary none of the rest would even be necessary.

> -	conn_state = (VolumeStateType)*(int32_t *)ptr;
> +/* avoid unaligned memory accesses */
> +        { int32_t temp;
> +          memcpy(&temp, ptr, sizeof(int32_t));
> +          conn_state = (VolumeStateType)temp;
> +        }
> +/*	conn_state = (VolumeStateType)*(int32_t *)ptr; */

I am somewhat surprised you aren't getting the same alignment errors
when this is packed on the venus side, because the code there is pretty
much identical and even has a comment about possible alignment issues.

    /* do we have to worry about alignment? */
    *(int32_t *)cp = (int32_t)conn_state; cp += sizeof(int32_t);
    ...

This just needs to be fixed by padding the strings out.

I can't believe this was the only place that had an alignment problem
like this, those ioctls are badly packed all over the place.

Jan
Received on 2016-04-22 11:56:01