Re: Fix netbsd32's getfh()

[christos%astron.com@localhost (Christos Zoulas)]

In article <53AA91A3.1070300%M00nBSD.net@localhost>,
Maxime Villard  <max%M00nBSD.net@localhost> wrote:
>Hi,
>Here is a patch to sync netbsd32 with the native getfh() syscall. In addition
>to making it consistent, it also:
>
>a) fixes the return value:
>               } else if (error == E2BIG) {
>                       error = copyout(&sz, SCARG_P32(uap, fh_size), 
> sizeof(size_t));
>               }
>   here the error code is overwritten by copyout(), so it won't ever return
>   E2BIG
>
>b) fixes a leak:
>               if (fh == NULL) 
>                       return EINVAL;
>   a vput(vp) is missing here
>
>c) fixes a user-controlled allocation:
>               fh = kmem_alloc(sz32, KM_SLEEP);
>
>I would like some ok's before committing it. Tested on NetBSD-current/amd64.
>
>Thanks.
>
>
>Index: netbsd32_netbsd.c
>===================================================================
>RCS file: /cvsroot/src/sys/compat/netbsd32/netbsd32_netbsd.c,v
>retrieving revision 1.190
>diff -u -r1.190 netbsd32_netbsd.c
>--- netbsd32_netbsd.c  22 Jun 2014 19:09:39 -0000      1.190
>+++ netbsd32_netbsd.c  25 Jun 2014 07:21:23 -0000
>@@ -1302,7 +1302,7 @@
>       int error;
>       struct pathbuf *pb;
>       struct nameidata nd;
>-      netbsd32_size_t sz32;
>+      netbsd32_size_t usz32, sz32;
>       size_t sz;
> 
>       /*
>@@ -1312,7 +1312,6 @@
>           0, NULL, NULL, NULL);
>       if (error)
>               return (error);
>-      fh = NULL;
> 
>       error = pathbuf_copyin(SCARG_P32(uap, fname), &pb);
>       if (error) {
>@@ -1328,30 +1327,31 @@
>       vp = nd.ni_vp;
>       pathbuf_destroy(pb);
> 
>-      error = copyin(SCARG_P32(uap, fh_size), &sz32,
>+      error = vfs_composefh_alloc(vp, &fh);
>+      vput(vp);
>+      if (error != 0) {
>+              goto out;

That should probably be:

                return error;

since vfs_composefh_alloc() failed, and we should not be calling 
vfs_composefh_free().

>+      }
>+      error = copyin(SCARG_P32(uap, fh_size), &usz32,
>           sizeof(netbsd32_size_t));

I would change sizeof(netbsd32_size_t) sizeof(usz32), that makes it
fit on one line too :-)

>-      if (error) {
>-              vput(vp);
>-              return error;
>+      if (error != 0) {
>+              goto out;
>       }
>-      fh = kmem_alloc(sz32, KM_SLEEP);
>-      if (fh == NULL) 
>-              return EINVAL;
>-      sz = sz32;
>-      error = vfs_composefh(vp, fh, &sz);
>-      vput(vp);
>+      sz = FHANDLE_SIZE(fh);
>+      sz32 = sz;
> 
>-      if (error == 0) {
>-              const netbsd32_size_t nsz32 = sz;
>-              error = copyout(&nsz32, SCARG_P32(uap, fh_size),
>-                  sizeof(netbsd32_size_t));
>-              if (!error) {
>-                      error = copyout(fh, SCARG_P32(uap, fhp), sz);
>-              }
>-      } else if (error == E2BIG) {
>-              error = copyout(&sz, SCARG_P32(uap, fh_size), sizeof(size_t));
>+      error = copyout(&sz32, SCARG_P32(uap, fh_size),
>+          sizeof(netbsd32_size_t));

Again perhaps sizeof(sz32)...

christos

>+      if (error != 0) {
>+              goto out;
>+      }
>+      if (usz32 >= sz32) {
>+              error = copyout(fh, SCARG_P32(uap, fhp), sz);
>+      } else {
>+              error = E2BIG;
>       }
>-      kmem_free(fh, sz32);
>+out:
>+      vfs_composefh_free(fh);
>       return (error);
> }
> 
>