Bug#102437: dealloc analysis for each server function, more bugs!

[Marcus Brinkmann]

Hi,

ok, here a detailed analysis.  It seems I found four other bugs along the
way. ;)

On Wed, Jun 27, 2001 at 12:35:25PM +0200, Marcus Brinkmann wrote:
>   routine proc_getprocinfo (
>           process: process_t;
>           which: pid_t;
>           inout flags: int;
> -         out procinfo: procinfo_t;
> +         out procinfo: procinfo_t, dealloc;
>           out threadwaits: data_t, dealloc);

proc_getprocinfo uses mmap if the buffer is not big enough, and copies the
out data into it.  This one definitely needs a dealloc.

>   routine file_get_storage_info (
>           file: file_t;
>           RPT
> -         out ports: portarray_t;
> -         out ints: intarray_t;          
> -         out offsets: off_array_t;
> -         out data: data_t);
> +         out ports: portarray_t, dealloc;
> +         out ints: intarray_t, dealloc;
> +         out offsets: off_array_t, dealloc;
> +         out data: data_t, dealloc);

store_encode (used by ext2fs, ufs mmaps new buffers if the provided ones
are not big enough.  Other callers I checked (default in libnetfs) also
do this.  So, all these should get dealloc, too.

BTW, the comment in libstore enc.c seems to be wrong:

/* Copy out the parameters from ENC into the given variables suitably for
   returning from a file_get_storage_info rpc, and deallocate ENC.  */
void
store_enc_return (struct store_enc *enc,
 
ENC is not allocated, and it is not deallocated either.

>   routine file_get_fs_options (
>           file: file_t;
>           RPT
> -         out options: data_t);
> +         out options: data_t, dealloc);

This one uses iohelp_return_malloced_buffer in libdiskfs/libnetfs/libtrivfs,
which mmaps a new buffer if the provided one is not big enough and copies
the string.  So, dealloc!

>   routine fsys_get_options (
>           server: fsys_t;
>           RPT
> -         out options: data_t);
> +         out options: data_t, dealloc);

See file_get_fs_options, the code is very similar.
 
>   routine auth_getids (
>           handle: auth_t;
> -         out euids: idarray_t;
> -         out auids: idarray_t;
> -         out egids: idarray_t;
> -         out agids: idarray_t);
> +         out euids: idarray_t, dealloc;
> +         out auids: idarray_t, dealloc;
> +         out egids: idarray_t, dealloc;
> +         out agids: idarray_t, dealloc);

This one is questionable.  auth_getids uses idvec_copyout, which provides
the internal buffer:

inline void idvec_copyout (struct idvec *idvec, uid_t **ids, uid_t *nids)
{
  if (idvec->num > *nids)
    *ids = idvec->ids;
  else
    memcpy (*ids, idvec->ids, idvec->num * sizeof *ids);
  *nids = idvec->num;
}

So, this should not use dealloc.  However, idvec->ids is NOT a mmap'ed
buffer!  It is a malloced/realloced buffer (coming from
libshouldbeinlibc/idvec.c).  Also, the code should not use
sizeof *ids, but sizeof (uid_t) = sizeof **ids for the calculation!

Is it correct that this should better use iohelp_return_malloced_buffer
and dealloc or mmap in idvec.h for the performance?

>   routine auth_server_authenticate (
>           handle: auth_t;
>           sreplyport reply: mach_port_poly_t;
>           rendezvous: mach_port_send_t;
>           newport: mach_port_poly_t;
> -         out euids: idarray_t;
> -         out auids: idarray_t;
> -         out egids: idarray_t;
> -         out agids: idarray_t);
> +         out euids: idarray_t, dealloc;
> +         out auids: idarray_t, dealloc;
> +         out egids: idarray_t, dealloc;
> +         out agids: idarray_t, dealloc);


Again, I am not sure.  The server routine has:

  /* Extract the ids.  We must use a separate reply stub so
     we can deref the user auth handle after the reply uses its
     contents.  */
  auth_server_authenticate_reply (reply, reply_type, 0,
                                  user->euids.ids, user->euids.num,
                                  user->auids.ids, user->auids.num,
                                  user->egids.ids, user->egids.num,
                                  user->agids.ids, user->agids.num);
  ports_port_deref (user);

Because those arrays are not marked as dealloc, the pointer is copied by
the reply stub.  Again, we are returning malloc'ed buffers.  But this time
we are returning buffers without keeping a reference to them!  The comment
is actually useless, as the buffers are not copied.  Am I incorrect, or is
this a blatant error?

It seems that using return-buffer and dealloc is the only chance, if we
don't want to giver the server a reference to the user.

Thanks,
Marcus

-- 
`Rhubarb is no Egyptian god.' Debian http://www.debian.org brinkmd@debian.org
Marcus Brinkmann              GNU    http://www.gnu.org    marcus@gnu.org
Marcus.Brinkmann@ruhr-uni-bochum.de
http://www.marcus-brinkmann.de