Re: svn commit: r279932 - head/sys/vm
On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote: On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: On Thu, 2015-03-12 at 17:02 -0400, Ryan Stone wrote: On Thu, Mar 12, 2015 at 2:06 PM, Ian Lepore i...@freebsd.org wrote: Nullterminate strings returned via sysctl. PR: 195668 To quote the manpage: The *sbuf* family of functions allows one to safely allocate, construct and release bounded null-terminated strings in kernel space. IMO the sbuf API is broken if we have to explicitly null-terminate the string ourselves. If we want the nullterm to be counted in the length of data in the buffer (and thus get transmitted back across the syscall boundary) we need to put an explicit counted nullterm byte into the buffer. I had started down the path of making that (counting the nullterm byte as part of the data in the buffer) a feature of sbuf that you could set with a flag, but then whoever added sbuf_new_for_sysctl() didn't propagate the flags field through the new function and I decided to not go off into the weeds making a new flavor of that takes flags. One suggestion would be to consider using '\0' for a nul character instead of a bare 0. To me that communicates the intention more clearly to the reader. (One of the things I did not like about C++ C++11 was the use of 0 for NULL. I much prefer nullptr and NULL in C over bare 0's for pointers for similar clarity reasons.) I have waffled back and forth between preferring 0 or '\0' for 30 years, I just seem to go through phases with nullterm expression. In general I'm glad I got called away to an onsite meeting yesterday and didn't get far with these changes, because the more I think about it, the less satisfied I am with this expedient fix. The other fix I started on, where a new SBUF_COUNTNUL flag can be set to inform the sbuf_finish() code that you want the terminating nul counted in the data length just feels like a better fit for the overall automaticness of how the sbuf stuff works. Hmm, I actually think that it's a bug that the terminating nul isn't included when draining. If we fixed that then I think that fixes most of these? The places that explicitly use 'sysctl_handle_string()' with an sbuf should probably just be using sbuf_len(sb) + 1' explicitly. (Another option would be to have a sysctl_handle_sbuf() that was a wrapper around sysctl_handle_string() that included the + 1 to hide that detail if there is more than one.) -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Fri, 2015-03-13 at 13:19 -0400, John Baldwin wrote: On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote: On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: [...] In general I'm glad I got called away to an onsite meeting yesterday and didn't get far with these changes, because the more I think about it, the less satisfied I am with this expedient fix. The other fix I started on, where a new SBUF_COUNTNUL flag can be set to inform the sbuf_finish() code that you want the terminating nul counted in the data length just feels like a better fit for the overall automaticness of how the sbuf stuff works. Hmm, I actually think that it's a bug that the terminating nul isn't included when draining. If we fixed that then I think that fixes most of these? The places that explicitly use 'sysctl_handle_string()' with an sbuf should probably just be using sbuf_len(sb) + 1' explicitly. (Another option would be to have a sysctl_handle_sbuf() that was a wrapper around sysctl_handle_string() that included the + 1 to hide that detail if there is more than one.) Some of the uses of sbuf for sysctl use sbuf_bcat() for dealing with binary structs, so we can't just assume that a nullterm should be added and included in the buffer length -- there needs to be some mechanism to say explicitly this is an sbuf for a sysctl string (and more generally this is an sbuf for a string where I want the nul byte counted as part of the data because that could be useful in non-sysctl contexts too, especially in userland). -- Ian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Friday, March 13, 2015 02:42:13 PM Ian Lepore wrote: On Fri, 2015-03-13 at 14:34 -0400, John Baldwin wrote: On Friday, March 13, 2015 11:57:58 AM Ian Lepore wrote: On Fri, 2015-03-13 at 13:19 -0400, John Baldwin wrote: On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote: On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: [...] In general I'm glad I got called away to an onsite meeting yesterday and didn't get far with these changes, because the more I think about it, the less satisfied I am with this expedient fix. The other fix I started on, where a new SBUF_COUNTNUL flag can be set to inform the sbuf_finish() code that you want the terminating nul counted in the data length just feels like a better fit for the overall automaticness of how the sbuf stuff works. Hmm, I actually think that it's a bug that the terminating nul isn't included when draining. If we fixed that then I think that fixes most of these? The places that explicitly use 'sysctl_handle_string()' with an sbuf should probably just be using sbuf_len(sb) + 1' explicitly. (Another option would be to have a sysctl_handle_sbuf() that was a wrapper around sysctl_handle_string() that included the + 1 to hide that detail if there is more than one.) Some of the uses of sbuf for sysctl use sbuf_bcat() for dealing with binary structs, so we can't just assume that a nullterm should be added and included in the buffer length -- there needs to be some mechanism to say explicitly this is an sbuf for a sysctl string (and more generally this is an sbuf for a string where I want the nul byte counted as part of the data because that could be useful in non-sysctl contexts too, especially in userland). Humm, that would seem to be an abuse of the API really. It is specifically designed for strings as someone else noted at the start of this thread (and as noted in the manpage). If anything I'd argue that the use cases that don't want a string should be the ones that should get a special flag in that case (or perhaps we should have a different little API to manage a buffer used for a draining sysctl where the data is a binary blob instead of a string). If you agree I'm happy to do some of the work (e.g. the different wrapper API). Given the existance of sbuf_bcpy() and sbuf_bcat() I'm not sure we can say using sbuf for binary data is any kind of violation; somebody just used the API that was provided to solve their problem. Well, it still nul-terminates the result of those, so I think it's still really dealing with strings. Those can be useful for appending non nul-terminated strings (for example using the d_namelen from a dirent). However, I think an INCLUDENUL flag is fine. It is a smaller change than adding a new sysctl-drain API. Binary data is the exception in the sysctl case, and the idea of having sbuf_new_for_sysctl() assume you're setting up to handle a sysctl string and requiring the rare binary uses to do something different does make a lot of sense. That might lead to a patch like the one below, which would automatically fix most of the current sysctl sbuf users, and the 2 or 3 places that are using binary data would need to add a line: sbuf_clear_flags(sbuf, SBUF_INCLUDENUL); I should mention too that the larger problem I'm trying to clean up is that some sysctl strings include the nul byte in the data returned to userland and some don't. There are more direct callers of SYSCTL_OUT() that fail to add a nulterm, I have a whole separate set of fixes for those, but I'm becoming somewhat inclined to fix them by converting them to use sbuf and just make that the established idiom for returning dynamic strings via sysctl. Either that or using sysctl_handle_string() when possible instead of bare SYSCTL_OUT(). I think your patch is fine. I agree this will be a much smaller change to roll out. :) -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Friday, March 13, 2015 11:57:58 AM Ian Lepore wrote: On Fri, 2015-03-13 at 13:19 -0400, John Baldwin wrote: On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote: On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: [...] In general I'm glad I got called away to an onsite meeting yesterday and didn't get far with these changes, because the more I think about it, the less satisfied I am with this expedient fix. The other fix I started on, where a new SBUF_COUNTNUL flag can be set to inform the sbuf_finish() code that you want the terminating nul counted in the data length just feels like a better fit for the overall automaticness of how the sbuf stuff works. Hmm, I actually think that it's a bug that the terminating nul isn't included when draining. If we fixed that then I think that fixes most of these? The places that explicitly use 'sysctl_handle_string()' with an sbuf should probably just be using sbuf_len(sb) + 1' explicitly. (Another option would be to have a sysctl_handle_sbuf() that was a wrapper around sysctl_handle_string() that included the + 1 to hide that detail if there is more than one.) Some of the uses of sbuf for sysctl use sbuf_bcat() for dealing with binary structs, so we can't just assume that a nullterm should be added and included in the buffer length -- there needs to be some mechanism to say explicitly this is an sbuf for a sysctl string (and more generally this is an sbuf for a string where I want the nul byte counted as part of the data because that could be useful in non-sysctl contexts too, especially in userland). Humm, that would seem to be an abuse of the API really. It is specifically designed for strings as someone else noted at the start of this thread (and as noted in the manpage). If anything I'd argue that the use cases that don't want a string should be the ones that should get a special flag in that case (or perhaps we should have a different little API to manage a buffer used for a draining sysctl where the data is a binary blob instead of a string). If you agree I'm happy to do some of the work (e.g. the different wrapper API). -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Fri, 2015-03-13 at 14:34 -0400, John Baldwin wrote:
On Friday, March 13, 2015 11:57:58 AM Ian Lepore wrote:
On Fri, 2015-03-13 at 13:19 -0400, John Baldwin wrote:
On Friday, March 13, 2015 10:14:27 AM Ian Lepore wrote:
On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote:
On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote:
[...]
In general I'm glad I got called away to an onsite meeting yesterday and
didn't get far with these changes, because the more I think about it,
the less satisfied I am with this expedient fix. The other fix I
started on, where a new SBUF_COUNTNUL flag can be set to inform the
sbuf_finish() code that you want the terminating nul counted in the data
length just feels like a better fit for the overall automaticness of
how the sbuf stuff works.
Hmm, I actually think that it's a bug that the terminating nul isn't
included
when draining. If we fixed that then I think that fixes most of these?
The places that explicitly use 'sysctl_handle_string()' with an sbuf
should probably just be using sbuf_len(sb) + 1' explicitly. (Another
option would be to have a sysctl_handle_sbuf() that was a wrapper around
sysctl_handle_string() that included the + 1 to hide that detail if there
is
more than one.)
Some of the uses of sbuf for sysctl use sbuf_bcat() for dealing with
binary structs, so we can't just assume that a nullterm should be added
and included in the buffer length -- there needs to be some mechanism to
say explicitly this is an sbuf for a sysctl string (and more generally
this is an sbuf for a string where I want the nul byte counted as part
of the data because that could be useful in non-sysctl contexts too,
especially in userland).
Humm, that would seem to be an abuse of the API really. It is specifically
designed for strings as someone else noted at the start of this thread (and
as noted in the manpage). If anything I'd argue that the use cases that don't
want a string should be the ones that should get a special flag in that case
(or perhaps we should have a different little API to manage a buffer used for
a draining sysctl where the data is a binary blob instead of a string). If
you agree I'm happy to do some of the work (e.g. the different wrapper API).
Given the existance of sbuf_bcpy() and sbuf_bcat() I'm not sure we can
say using sbuf for binary data is any kind of violation; somebody just
used the API that was provided to solve their problem.
Binary data is the exception in the sysctl case, and the idea of having
sbuf_new_for_sysctl() assume you're setting up to handle a sysctl string
and requiring the rare binary uses to do something different does make a
lot of sense. That might lead to a patch like the one below, which
would automatically fix most of the current sysctl sbuf users, and the 2
or 3 places that are using binary data would need to add a line:
sbuf_clear_flags(sbuf, SBUF_INCLUDENUL);
I should mention too that the larger problem I'm trying to clean up is
that some sysctl strings include the nul byte in the data returned to
userland and some don't. There are more direct callers of SYSCTL_OUT()
that fail to add a nulterm, I have a whole separate set of fixes for
those, but I'm becoming somewhat inclined to fix them by converting them
to use sbuf and just make that the established idiom for returning
dynamic strings via sysctl.
-- Ian
Index: sys/kern/kern_sysctl.c
===
--- sys/kern/kern_sysctl.c (revision 279962)
+++ sys/kern/kern_sysctl.c (working copy)
@@ -1807,7 +1807,7 @@ sbuf_new_for_sysctl(struct sbuf *s, char *buf, int
struct sysctl_req *req)
{
- s = sbuf_new(s, buf, length, SBUF_FIXEDLEN);
+ s = sbuf_new(s, buf, length, SBUF_FIXEDLEN | SBUF_INCLUDENUL);
sbuf_set_drain(s, sbuf_sysctl_drain, req);
return (s);
}
Index: sys/kern/subr_sbuf.c
===
--- sys/kern/subr_sbuf.c (revision 279962)
+++ sys/kern/subr_sbuf.c (working copy)
@@ -262,6 +262,28 @@ sbuf_uionew(struct sbuf *s, struct uio *uio, int *
}
#endif
+int
+sbuf_get_flags(struct sbuf *s)
+{
+
+ return (s-s_flags);
+}
+
+void
+sbuf_clear_flags(struct sbuf *s, int flags)
+{
+
+ s-s_flags = ~(flags SBUF_USRFLAGMSK);
+}
+
+void
+sbuf_set_flags(struct sbuf *s, int flags)
+{
+
+
+ s-s_flags |= (flags SBUF_USRFLAGMSK);
+}
+
/*
* Clear an sbuf and reset its position.
*/
@@ -697,11 +719,13 @@ sbuf_finish(struct sbuf *s)
assert_sbuf_integrity(s);
assert_sbuf_state(s, 0);
+ s-s_buf[s-s_len] = '\0';
+ if (s-s_flags SBUF_INCLUDENUL)
+ s-s_len++;
if (s-s_drain_func != NULL) {
while (s-s_len 0 s-s_error == 0)
s-s_error = sbuf_drain(s);
}
- s-s_buf[s-s_len] = '\0';
SBUF_SETFLAG(s, SBUF_FINISHED);
#ifdef _KERNEL
return (s-s_error);
@@ -743,6 +767,10 @@ sbuf_len(struct sbuf *s)
if (s-s_error != 0)
return
Re: svn commit: r279932 - head/sys/vm
On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: On Thu, 2015-03-12 at 17:02 -0400, Ryan Stone wrote: On Thu, Mar 12, 2015 at 2:06 PM, Ian Lepore i...@freebsd.org wrote: Nullterminate strings returned via sysctl. PR: 195668 To quote the manpage: The *sbuf* family of functions allows one to safely allocate, construct and release bounded null-terminated strings in kernel space. IMO the sbuf API is broken if we have to explicitly null-terminate the string ourselves. If we want the nullterm to be counted in the length of data in the buffer (and thus get transmitted back across the syscall boundary) we need to put an explicit counted nullterm byte into the buffer. I had started down the path of making that (counting the nullterm byte as part of the data in the buffer) a feature of sbuf that you could set with a flag, but then whoever added sbuf_new_for_sysctl() didn't propagate the flags field through the new function and I decided to not go off into the weeds making a new flavor of that takes flags. One suggestion would be to consider using '\0' for a nul character instead of a bare 0. To me that communicates the intention more clearly to the reader. (One of the things I did not like about C++ C++11 was the use of 0 for NULL. I much prefer nullptr and NULL in C over bare 0's for pointers for similar clarity reasons.) -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Fri, 2015-03-13 at 06:24 -0400, John Baldwin wrote: On Thursday, March 12, 2015 05:24:51 PM Ian Lepore wrote: On Thu, 2015-03-12 at 17:02 -0400, Ryan Stone wrote: On Thu, Mar 12, 2015 at 2:06 PM, Ian Lepore i...@freebsd.org wrote: Nullterminate strings returned via sysctl. PR: 195668 To quote the manpage: The *sbuf* family of functions allows one to safely allocate, construct and release bounded null-terminated strings in kernel space. IMO the sbuf API is broken if we have to explicitly null-terminate the string ourselves. If we want the nullterm to be counted in the length of data in the buffer (and thus get transmitted back across the syscall boundary) we need to put an explicit counted nullterm byte into the buffer. I had started down the path of making that (counting the nullterm byte as part of the data in the buffer) a feature of sbuf that you could set with a flag, but then whoever added sbuf_new_for_sysctl() didn't propagate the flags field through the new function and I decided to not go off into the weeds making a new flavor of that takes flags. One suggestion would be to consider using '\0' for a nul character instead of a bare 0. To me that communicates the intention more clearly to the reader. (One of the things I did not like about C++ C++11 was the use of 0 for NULL. I much prefer nullptr and NULL in C over bare 0's for pointers for similar clarity reasons.) I have waffled back and forth between preferring 0 or '\0' for 30 years, I just seem to go through phases with nullterm expression. In general I'm glad I got called away to an onsite meeting yesterday and didn't get far with these changes, because the more I think about it, the less satisfied I am with this expedient fix. The other fix I started on, where a new SBUF_COUNTNUL flag can be set to inform the sbuf_finish() code that you want the terminating nul counted in the data length just feels like a better fit for the overall automaticness of how the sbuf stuff works. -- Ian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Thu, Mar 12, 2015 at 2:06 PM, Ian Lepore i...@freebsd.org wrote: Nullterminate strings returned via sysctl. PR: 195668 To quote the manpage: The *sbuf* family of functions allows one to safely allocate, construct and release bounded null-terminated strings in kernel space. IMO the sbuf API is broken if we have to explicitly null-terminate the string ourselves. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
svn commit: r279932 - head/sys/vm
Author: ian Date: Thu Mar 12 18:06:30 2015 New Revision: 279932 URL: https://svnweb.freebsd.org/changeset/base/279932 Log: Nullterminate strings returned via sysctl. PR: 195668 Modified: head/sys/vm/vm_phys.c head/sys/vm/vm_reserv.c Modified: head/sys/vm/vm_phys.c == --- head/sys/vm/vm_phys.c Thu Mar 12 17:10:04 2015(r279931) +++ head/sys/vm/vm_phys.c Thu Mar 12 18:06:30 2015(r279932) @@ -263,6 +263,7 @@ sysctl_vm_phys_free(SYSCTL_HANDLER_ARGS) } } } + sbuf_putc(sbuf, 0); /* nullterm */ error = sbuf_finish(sbuf); sbuf_delete(sbuf); return (error); @@ -292,6 +293,7 @@ sysctl_vm_phys_segs(SYSCTL_HANDLER_ARGS) sbuf_printf(sbuf, domain:%d\n, seg-domain); sbuf_printf(sbuf, free list: %p\n, seg-free_queues); } + sbuf_putc(sbuf, 0); /* nullterm */ error = sbuf_finish(sbuf); sbuf_delete(sbuf); return (error); Modified: head/sys/vm/vm_reserv.c == --- head/sys/vm/vm_reserv.c Thu Mar 12 17:10:04 2015(r279931) +++ head/sys/vm/vm_reserv.c Thu Mar 12 18:06:30 2015(r279932) @@ -261,6 +261,7 @@ sysctl_vm_reserv_partpopq(SYSCTL_HANDLER sbuf_printf(sbuf, %5d: %6dK, %6d\n, level, unused_pages * ((int)PAGE_SIZE / 1024), counter); } + sbuf_putc(sbuf, 0); /* nullterm */ error = sbuf_finish(sbuf); sbuf_delete(sbuf); return (error); ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r279932 - head/sys/vm
On Thu, 2015-03-12 at 17:02 -0400, Ryan Stone wrote: On Thu, Mar 12, 2015 at 2:06 PM, Ian Lepore i...@freebsd.org wrote: Nullterminate strings returned via sysctl. PR: 195668 To quote the manpage: The *sbuf* family of functions allows one to safely allocate, construct and release bounded null-terminated strings in kernel space. IMO the sbuf API is broken if we have to explicitly null-terminate the string ourselves. If we want the nullterm to be counted in the length of data in the buffer (and thus get transmitted back across the syscall boundary) we need to put an explicit counted nullterm byte into the buffer. I had started down the path of making that (counting the nullterm byte as part of the data in the buffer) a feature of sbuf that you could set with a flag, but then whoever added sbuf_new_for_sysctl() didn't propagate the flags field through the new function and I decided to not go off into the weeds making a new flavor of that takes flags. -- Ian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
