Re: Fix netbsd32's getfh()
Le 26/06/2014 20:46, Christos Zoulas a écrit : On Jun 26, 4:30pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: Fix netbsd32's getfh() | I guess it's ok now? Yes, that looks fine. But why: if (error != 0) instead of: if (error) I think that most code in the tree uses the second form. But the native code doesn't. Anyway, I've committed the fix. Thanks.
Re: Fix netbsd32's getfh()
Le 26/06/2014 04:04, Christos Zoulas a écrit :
On Jun 26, 11:30am, m...@eterna.com.au (matthew green) wrote:
-- Subject: re: Fix netbsd32's getfh()
|
| Christos Zoulas writes:
| Well, let's not propagate the evil design! I is is one thing having:
|
|void *p = malloc(n);
|free(p);
|
| where you know p is going to be NULL when it fails and another having:
|
|foo *p;
|error = allocate_foo(p);
|free_foo(p);
|
| and expect p to be altered in the error path of allocate_foo.
|
| Please, let's fix it! I volunteer :-)
|
| *tag* you're it!
Done.
christos
I guess it's ok now?
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 - 1.190
+++ netbsd32_netbsd.c 26 Jun 2014 14:21:24 -
@@ -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,29 @@
vp = nd.ni_vp;
pathbuf_destroy(pb);
- error = copyin(SCARG_P32(uap, fh_size), sz32,
- sizeof(netbsd32_size_t));
- if (error) {
- vput(vp);
+ error = vfs_composefh_alloc(vp, fh);
+ vput(vp);
+ if (error != 0) {
return error;
}
- fh = kmem_alloc(sz32, KM_SLEEP);
- if (fh == NULL)
- return EINVAL;
- sz = sz32;
- error = vfs_composefh(vp, fh, sz);
- vput(vp);
+ error = copyin(SCARG_P32(uap, fh_size), usz32, sizeof(usz32));
+ if (error != 0) {
+ goto out;
+ }
+ 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(sz32));
+ 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);
}
Re: Fix netbsd32's getfh()
On Jun 26, 4:30pm, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: Fix netbsd32's getfh()
| I guess it's ok now?
Yes, that looks fine. But why:
if (error != 0)
instead of:
if (error)
I think that most code in the tree uses the second form.
christos
|
| 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 - 1.190
| +++ netbsd32_netbsd.c 26 Jun 2014 14:21:24 -
| @@ -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,29 @@
| vp = nd.ni_vp;
| pathbuf_destroy(pb);
|
| - error = copyin(SCARG_P32(uap, fh_size), sz32,
| - sizeof(netbsd32_size_t));
| - if (error) {
| - vput(vp);
| + error = vfs_composefh_alloc(vp, fh);
| + vput(vp);
| + if (error != 0) {
| return error;
| }
| - fh = kmem_alloc(sz32, KM_SLEEP);
| - if (fh == NULL)
| - return EINVAL;
| - sz = sz32;
| - error = vfs_composefh(vp, fh, sz);
| - vput(vp);
| + error = copyin(SCARG_P32(uap, fh_size), usz32, sizeof(usz32));
| + if (error != 0) {
| + goto out;
| + }
| + 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(sz32));
| + 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);
| }
|
-- End of excerpt from Maxime Villard
Re: Fix netbsd32's getfh()
In article 53aa91a3.1070...@m00nbsd.net,
Maxime Villard m...@m00nbsd.net 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 - 1.190
+++ netbsd32_netbsd.c 25 Jun 2014 07:21:23 -
@@ -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);
}
Re: Fix netbsd32's getfh()
Le 25/06/2014 17:28, Christos Zoulas a écrit :
In article 53aa91a3.1070...@m00nbsd.net,
Maxime Villard m...@m00nbsd.net 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.c22 Jun 2014 19:09:39 - 1.190
+++ netbsd32_netbsd.c25 Jun 2014 07:21:23 -
@@ -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().
Yes, but no. Actually, vfs_composefh_free() handles NULL correctly,
and that's what the native code does. I know it's an evil design.
+}
+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 :-)
Yep
[...]
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 - 1.190
+++ netbsd32_netbsd.c 25 Jun 2014 17:11:15 -
@@ -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,29 @@
vp = nd.ni_vp;
pathbuf_destroy(pb);
- error = copyin(SCARG_P32(uap, fh_size), sz32,
- sizeof(netbsd32_size_t));
- if (error) {
- vput(vp);
- return error;
- }
- fh = kmem_alloc(sz32, KM_SLEEP);
- if (fh == NULL)
- return EINVAL;
- sz = sz32;
- error = vfs_composefh(vp, fh, sz);
+ error = vfs_composefh_alloc(vp, fh);
vput(vp);
+ if (error != 0) {
+ goto out;
+ }
+ error = copyin(SCARG_P32(uap, fh_size), usz32, sizeof(usz32));
+ if (error != 0) {
+ goto out;
+ }
+ 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(sz32));
+ 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);
}
Re: Fix netbsd32's getfh()
In article 53ab0471.4080...@m00nbsd.net, Maxime Villard m...@m00nbsd.net wrote: That should probably be: return error; since vfs_composefh_alloc() failed, and we should not be calling vfs_composefh_free(). Yes, but no. Actually, vfs_composefh_free() handles NULL correctly, and that's what the native code does. I know it's an evil design. Well, let's not propagate the evil design! I is is one thing having: void *p = malloc(n); free(p); where you know p is going to be NULL when it fails and another having: foo *p; error = allocate_foo(p); free_foo(p); and expect p to be altered in the error path of allocate_foo. Please, let's fix it! I volunteer :-) christos
re: Fix netbsd32's getfh()
Christos Zoulas writes: Well, let's not propagate the evil design! I is is one thing having: void *p = malloc(n); free(p); where you know p is going to be NULL when it fails and another having: foo *p; error = allocate_foo(p); free_foo(p); and expect p to be altered in the error path of allocate_foo. Please, let's fix it! I volunteer :-) *tag* you're it!
re: Fix netbsd32's getfh()
On Jun 26, 11:30am, m...@eterna.com.au (matthew green) wrote: -- Subject: re: Fix netbsd32's getfh() | | Christos Zoulas writes: | Well, let's not propagate the evil design! I is is one thing having: | | void *p = malloc(n); | free(p); | | where you know p is going to be NULL when it fails and another having: | | foo *p; | error = allocate_foo(p); | free_foo(p); | | and expect p to be altered in the error path of allocate_foo. | | Please, let's fix it! I volunteer :-) | | *tag* you're it! Done. christos
