Re: svn commit: r367701 - head/lib/libutil
On Sun, Nov 15, 2020 at 1:13 PM Scott Long wrote: > > > On Nov 15, 2020, at 1:05 PM, Warner Losh wrote: > > > > Hey Scott, > > > > On Sun, Nov 15, 2020 at 11:46 AM Scott Long wrote: > > The man page for strlcpy() made reference to the return value being > > equivalent to what snprintf() does. The man page for snprintf() states > > that negatve return values are possible, so I assumed the same was > > true for strlcpy(). However, now that I’ve looked at the implementation > > of strlcpy(), I see that you’re correct. The man pages are definitely > > confusing, and this isn’t the only place where I think there’s > > inconsistency in the documentation, or at least poor wording choices. > > > > Yea, it says both that it will never return a negative value (since > size_t is never negative) and that it returns the same things as snprintf > (which is true... except for that detail which it glosses over in return > type differences). > > > > So this issue doesn't get lost, I've added a clarification to the > examples in https://reviews.freebsd.org/D27228 . Please take a look and > let me know what you think. If more extensive edits are needed, there's > full context so you can at least flag those in the review as well. I've > read these too many times to see the other places you're talking about, so > a fresh set of eyes would be helpful. > > > > The wording on whether or not strlcpy and strlcat will provide NULL > termination is also inconsistent, hence my comments about it last weekend. > I’m going to revert all of this back to and including r367075, since Stefan > wants to do this a totally different way. Sorry for the noise everyone and > thanks for the help, I learned a lot through this process. > OK. I'll update the man page to document the guaranteed behavior wrt NUL as well. I know what it's trying to say, but you are not the first person to stumble on these details. Thanks for pointing me at the ones that need improvement. And thanks for getting the localbase issue seen through. Warner ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
> On Nov 15, 2020, at 1:05 PM, Warner Losh wrote: > > Hey Scott, > > On Sun, Nov 15, 2020 at 11:46 AM Scott Long wrote: > The man page for strlcpy() made reference to the return value being > equivalent to what snprintf() does. The man page for snprintf() states > that negatve return values are possible, so I assumed the same was > true for strlcpy(). However, now that I’ve looked at the implementation > of strlcpy(), I see that you’re correct. The man pages are definitely > confusing, and this isn’t the only place where I think there’s > inconsistency in the documentation, or at least poor wording choices. > > Yea, it says both that it will never return a negative value (since size_t is > never negative) and that it returns the same things as snprintf (which is > true... except for that detail which it glosses over in return type > differences). > > So this issue doesn't get lost, I've added a clarification to the examples in > https://reviews.freebsd.org/D27228 . Please take a look and let me know what > you think. If more extensive edits are needed, there's full context so you > can at least flag those in the review as well. I've read these too many times > to see the other places you're talking about, so a fresh set of eyes would be > helpful. > The wording on whether or not strlcpy and strlcat will provide NULL termination is also inconsistent, hence my comments about it last weekend. I’m going to revert all of this back to and including r367075, since Stefan wants to do this a totally different way. Sorry for the noise everyone and thanks for the help, I learned a lot through this process. Scott ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
Hey Scott, On Sun, Nov 15, 2020 at 11:46 AM Scott Long wrote: > The man page for strlcpy() made reference to the return value being > equivalent to what snprintf() does. The man page for snprintf() states > that negatve return values are possible, so I assumed the same was > true for strlcpy(). However, now that I’ve looked at the implementation > of strlcpy(), I see that you’re correct. The man pages are definitely > confusing, and this isn’t the only place where I think there’s > inconsistency in the documentation, or at least poor wording choices. Yea, it says both that it will never return a negative value (since size_t is never negative) and that it returns the same things as snprintf (which is true... except for that detail which it glosses over in return type differences). So this issue doesn't get lost, I've added a clarification to the examples in https://reviews.freebsd.org/D27228 . Please take a look and let me know what you think. If more extensive edits are needed, there's full context so you can at least flag those in the review as well. I've read these too many times to see the other places you're talking about, so a fresh set of eyes would be helpful. Warner ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
That would explain why I see what I see -- I did not install an updated libc
yet.
On Sun, Nov 15, 2020, at 1:34 PM, Scott Long wrote:
> It is a magical namespace, in that it comes from libc, not from the
> kernel. Please make sure that you’ve installed a more recent libc, I
> guess? I just did a full build and install, and I’m unable to
> replicate the problem. Maybe there’s a static-linked pkg running
> around somewhere? I’m at a loss for better ideas.
>
> Scott
>
>
> > On Nov 15, 2020, at 12:30 PM, Brandon Bergren wrote:
> >
> > I think the problem is that user.* is somehow magically namespaced, so
> > doing a "dumb" sysctlbyname will get the wrong one.
> >
> > sysctl (the tool) does:
> > __sysctl("sysctl.name2oid
> > user.localbase",2,0xfbfffde98,0xfbfffda98,0x810809000,14) = 0 (0x0)
> > __sysctl("sysctl.oidfmt
> > user.localbase",4,0xfbfffdef8,0xfbfffd690,0x0,0) = 0 (0x0)
> > __sysctl("sysctl.name { 8.21 }",4,0xfbfffc8f8,0xfbfffc480,0x0,0) =
> > 0 (0x0)
> > __sysctl("sysctl.oidfmt
> > user.localbase",4,0xfbfffd0f8,0xfbfffc488,0x0,0) = 0 (0x0)
> > __sysctl("user.localbase",2,0x0,0xfbfffc480,0x0,0) = 0 (0x0)
> > __sysctl("user.localbase",2,0x81080a000,0xfbfffd0f8,0x0,0) = 0 (0x0)
> >
> > and picks up /usr/local.
> >
> > whereas libutil is currently just doing
> > __sysctlbyname("user.localbase",14,0xfbfffd4f8,0xfbfffd440,0x0,0) =
> > 0 (0x0)
> >
> > which is returning the builtin "" from the static kernel variable.
> >
> > On Sun, Nov 15, 2020, at 1:26 PM, Jessica Clarke wrote:
> >> On 15 Nov 2020, at 19:10, Brandon Bergren wrote:
> >>>
> >>> On powerpc64 and powerpc64le, there is some really weird behavior
> >>> happening around the sysctl itself:
> >>>
> >>> root@crow:~ # pkg
> >>> The package management tool is not yet installed on your system.
> >>> Do you want to fetch and install it now? [y/N]: N
> >>> root@crow:~ # sysctl user.localbase
> >>> user.localbase: /usr/local
> >>> root@crow:~ # pkg
> >>> The package management tool is not yet installed on your system.
> >>> Do you want to fetch and install it now? [y/N]: N
> >>> root@crow:~ # sysctl user.localbase=/usr/local
> >>> user.localbase: /usr/local -> /usr/local
> >>> root@crow:~ # pkg
> >>> pkg: not enough arguments
> >>> Usage: pkg [-v] [-d] [-l] [-N] [-j |-c |-r
> >>> ] [-C ] [-R ] [-o
> >>> var=value] [-4|-6] []
> >>>
> >>> For more information on available commands and options see 'pkg help'.
> >>> root@crow:~ #
> >>>
> >>>
> >>> I would double check very closely that the sysctl is being called
> >>> correctly, the sysctl tool manages to read it out, but libutil does not.
> >>
> >> That's odd. What does truss say?
> >>
> >> Jess
> >>
> >>> On Sun, Nov 15, 2020, at 1:06 PM, Scott Long wrote:
>
> > On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote:
> >>
> >> I felt similar concerns, but my misunderstanding of strlcpy() drove the
> >> result. Since the use case for getlocalbase() lends itself to also use
> >> strlcat()/strlcpy(), I was trying to replicate the API semantics of
> >> those,
> >> at least to the limit of my understanding. Thanks for the feedback,
> >> I’ll
> >> look at it some more.
> >
> > Thanks. ENOMEM also feels inappropriate as no allocation is taking
> > place. Perhaps ENAMETOOLONG, which is used in similar cases for things
> > like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh.
> >
>
> Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything
> better.
>
> > Also, if pathlen has already been checked against SSIZE_MAX (giving
> > EINVAL) and tmplen against pathlen there's no need to then check tmplen
> > against SSIZE_MAX.
> >
>
> Done.
>
> > I'd be happy to give a review on Phabricator if/when you have a new
> > patch.
> >
>
> https://reviews.freebsd.org/D27227
>
> Thanks,
> Scott
>
>
> >>>
> >>> --
> >>> Brandon Bergren
> >>> bdra...@freebsd.org
> >>
> >>
> >
> > --
> > Brandon Bergren
> > bdra...@freebsd.org
>
>
--
Brandon Bergren
bdra...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
It is a magical namespace, in that it comes from libc, not from the kernel.
Please make sure that you’ve installed a more recent libc, I guess? I just did
a full build and install, and I’m unable to replicate the problem. Maybe
there’s a static-linked pkg running around somewhere? I’m at a loss for better
ideas.
Scott
> On Nov 15, 2020, at 12:30 PM, Brandon Bergren wrote:
>
> I think the problem is that user.* is somehow magically namespaced, so doing
> a "dumb" sysctlbyname will get the wrong one.
>
> sysctl (the tool) does:
> __sysctl("sysctl.name2oid
> user.localbase",2,0xfbfffde98,0xfbfffda98,0x810809000,14) = 0 (0x0)
> __sysctl("sysctl.oidfmt
> user.localbase",4,0xfbfffdef8,0xfbfffd690,0x0,0) = 0 (0x0)
> __sysctl("sysctl.name { 8.21 }",4,0xfbfffc8f8,0xfbfffc480,0x0,0) = 0
> (0x0)
> __sysctl("sysctl.oidfmt
> user.localbase",4,0xfbfffd0f8,0xfbfffc488,0x0,0) = 0 (0x0)
> __sysctl("user.localbase",2,0x0,0xfbfffc480,0x0,0) = 0 (0x0)
> __sysctl("user.localbase",2,0x81080a000,0xfbfffd0f8,0x0,0) = 0 (0x0)
>
> and picks up /usr/local.
>
> whereas libutil is currently just doing
> __sysctlbyname("user.localbase",14,0xfbfffd4f8,0xfbfffd440,0x0,0) = 0
> (0x0)
>
> which is returning the builtin "" from the static kernel variable.
>
> On Sun, Nov 15, 2020, at 1:26 PM, Jessica Clarke wrote:
>> On 15 Nov 2020, at 19:10, Brandon Bergren wrote:
>>>
>>> On powerpc64 and powerpc64le, there is some really weird behavior happening
>>> around the sysctl itself:
>>>
>>> root@crow:~ # pkg
>>> The package management tool is not yet installed on your system.
>>> Do you want to fetch and install it now? [y/N]: N
>>> root@crow:~ # sysctl user.localbase
>>> user.localbase: /usr/local
>>> root@crow:~ # pkg
>>> The package management tool is not yet installed on your system.
>>> Do you want to fetch and install it now? [y/N]: N
>>> root@crow:~ # sysctl user.localbase=/usr/local
>>> user.localbase: /usr/local -> /usr/local
>>> root@crow:~ # pkg
>>> pkg: not enough arguments
>>> Usage: pkg [-v] [-d] [-l] [-N] [-j |-c |-r
>>> ] [-C ] [-R ] [-o var=value]
>>> [-4|-6] []
>>>
>>> For more information on available commands and options see 'pkg help'.
>>> root@crow:~ #
>>>
>>>
>>> I would double check very closely that the sysctl is being called
>>> correctly, the sysctl tool manages to read it out, but libutil does not.
>>
>> That's odd. What does truss say?
>>
>> Jess
>>
>>> On Sun, Nov 15, 2020, at 1:06 PM, Scott Long wrote:
> On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote:
>>
>> I felt similar concerns, but my misunderstanding of strlcpy() drove the
>> result. Since the use case for getlocalbase() lends itself to also use
>> strlcat()/strlcpy(), I was trying to replicate the API semantics of
>> those,
>> at least to the limit of my understanding. Thanks for the feedback, I’ll
>> look at it some more.
>
> Thanks. ENOMEM also feels inappropriate as no allocation is taking
> place. Perhaps ENAMETOOLONG, which is used in similar cases for things
> like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh.
>
Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything better.
> Also, if pathlen has already been checked against SSIZE_MAX (giving
> EINVAL) and tmplen against pathlen there's no need to then check tmplen
> against SSIZE_MAX.
>
Done.
> I'd be happy to give a review on Phabricator if/when you have a new
> patch.
>
https://reviews.freebsd.org/D27227
Thanks,
Scott
>>>
>>> --
>>> Brandon Bergren
>>> bdra...@freebsd.org
>>
>>
>
> --
> Brandon Bergren
> bdra...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
I think the problem is that user.* is somehow magically namespaced, so doing a
"dumb" sysctlbyname will get the wrong one.
sysctl (the tool) does:
__sysctl("sysctl.name2oid
user.localbase",2,0xfbfffde98,0xfbfffda98,0x810809000,14) = 0 (0x0)
__sysctl("sysctl.oidfmt
user.localbase",4,0xfbfffdef8,0xfbfffd690,0x0,0) = 0 (0x0)
__sysctl("sysctl.name { 8.21 }",4,0xfbfffc8f8,0xfbfffc480,0x0,0) = 0
(0x0)
__sysctl("sysctl.oidfmt
user.localbase",4,0xfbfffd0f8,0xfbfffc488,0x0,0) = 0 (0x0)
__sysctl("user.localbase",2,0x0,0xfbfffc480,0x0,0) = 0 (0x0)
__sysctl("user.localbase",2,0x81080a000,0xfbfffd0f8,0x0,0) = 0 (0x0)
and picks up /usr/local.
whereas libutil is currently just doing
__sysctlbyname("user.localbase",14,0xfbfffd4f8,0xfbfffd440,0x0,0) = 0
(0x0)
which is returning the builtin "" from the static kernel variable.
On Sun, Nov 15, 2020, at 1:26 PM, Jessica Clarke wrote:
> On 15 Nov 2020, at 19:10, Brandon Bergren wrote:
> >
> > On powerpc64 and powerpc64le, there is some really weird behavior happening
> > around the sysctl itself:
> >
> > root@crow:~ # pkg
> > The package management tool is not yet installed on your system.
> > Do you want to fetch and install it now? [y/N]: N
> > root@crow:~ # sysctl user.localbase
> > user.localbase: /usr/local
> > root@crow:~ # pkg
> > The package management tool is not yet installed on your system.
> > Do you want to fetch and install it now? [y/N]: N
> > root@crow:~ # sysctl user.localbase=/usr/local
> > user.localbase: /usr/local -> /usr/local
> > root@crow:~ # pkg
> > pkg: not enough arguments
> > Usage: pkg [-v] [-d] [-l] [-N] [-j |-c |-r
> > ] [-C ] [-R ] [-o var=value]
> > [-4|-6] []
> >
> > For more information on available commands and options see 'pkg help'.
> > root@crow:~ #
> >
> >
> > I would double check very closely that the sysctl is being called
> > correctly, the sysctl tool manages to read it out, but libutil does not.
>
> That's odd. What does truss say?
>
> Jess
>
> > On Sun, Nov 15, 2020, at 1:06 PM, Scott Long wrote:
> >>
> >>> On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote:
>
> I felt similar concerns, but my misunderstanding of strlcpy() drove the
> result. Since the use case for getlocalbase() lends itself to also use
> strlcat()/strlcpy(), I was trying to replicate the API semantics of
> those,
> at least to the limit of my understanding. Thanks for the feedback, I’ll
> look at it some more.
> >>>
> >>> Thanks. ENOMEM also feels inappropriate as no allocation is taking
> >>> place. Perhaps ENAMETOOLONG, which is used in similar cases for things
> >>> like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh.
> >>>
> >>
> >> Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything better.
> >>
> >>> Also, if pathlen has already been checked against SSIZE_MAX (giving
> >>> EINVAL) and tmplen against pathlen there's no need to then check tmplen
> >>> against SSIZE_MAX.
> >>>
> >>
> >> Done.
> >>
> >>> I'd be happy to give a review on Phabricator if/when you have a new
> >>> patch.
> >>>
> >>
> >> https://reviews.freebsd.org/D27227
> >>
> >> Thanks,
> >> Scott
> >>
> >>
> >
> > --
> > Brandon Bergren
> > bdra...@freebsd.org
>
>
--
Brandon Bergren
bdra...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
On 15 Nov 2020, at 19:10, Brandon Bergren wrote: > > On powerpc64 and powerpc64le, there is some really weird behavior happening > around the sysctl itself: > > root@crow:~ # pkg > The package management tool is not yet installed on your system. > Do you want to fetch and install it now? [y/N]: N > root@crow:~ # sysctl user.localbase > user.localbase: /usr/local > root@crow:~ # pkg > The package management tool is not yet installed on your system. > Do you want to fetch and install it now? [y/N]: N > root@crow:~ # sysctl user.localbase=/usr/local > user.localbase: /usr/local -> /usr/local > root@crow:~ # pkg > pkg: not enough arguments > Usage: pkg [-v] [-d] [-l] [-N] [-j |-c |-r > ] [-C ] [-R ] [-o var=value] > [-4|-6] [] > > For more information on available commands and options see 'pkg help'. > root@crow:~ # > > > I would double check very closely that the sysctl is being called correctly, > the sysctl tool manages to read it out, but libutil does not. That's odd. What does truss say? Jess > On Sun, Nov 15, 2020, at 1:06 PM, Scott Long wrote: >> >>> On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote: I felt similar concerns, but my misunderstanding of strlcpy() drove the result. Since the use case for getlocalbase() lends itself to also use strlcat()/strlcpy(), I was trying to replicate the API semantics of those, at least to the limit of my understanding. Thanks for the feedback, I’ll look at it some more. >>> >>> Thanks. ENOMEM also feels inappropriate as no allocation is taking >>> place. Perhaps ENAMETOOLONG, which is used in similar cases for things >>> like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh. >>> >> >> Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything better. >> >>> Also, if pathlen has already been checked against SSIZE_MAX (giving >>> EINVAL) and tmplen against pathlen there's no need to then check tmplen >>> against SSIZE_MAX. >>> >> >> Done. >> >>> I'd be happy to give a review on Phabricator if/when you have a new >>> patch. >>> >> >> https://reviews.freebsd.org/D27227 >> >> Thanks, >> Scott >> >> > > -- > Brandon Bergren > bdra...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
On powerpc64 and powerpc64le, there is some really weird behavior happening around the sysctl itself: root@crow:~ # pkg The package management tool is not yet installed on your system. Do you want to fetch and install it now? [y/N]: N root@crow:~ # sysctl user.localbase user.localbase: /usr/local root@crow:~ # pkg The package management tool is not yet installed on your system. Do you want to fetch and install it now? [y/N]: N root@crow:~ # sysctl user.localbase=/usr/local user.localbase: /usr/local -> /usr/local root@crow:~ # pkg pkg: not enough arguments Usage: pkg [-v] [-d] [-l] [-N] [-j |-c |-r ] [-C ] [-R ] [-o var=value] [-4|-6] [] For more information on available commands and options see 'pkg help'. root@crow:~ # I would double check very closely that the sysctl is being called correctly, the sysctl tool manages to read it out, but libutil does not. On Sun, Nov 15, 2020, at 1:06 PM, Scott Long wrote: > > > On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote: > >> > >> I felt similar concerns, but my misunderstanding of strlcpy() drove the > >> result. Since the use case for getlocalbase() lends itself to also use > >> strlcat()/strlcpy(), I was trying to replicate the API semantics of those, > >> at least to the limit of my understanding. Thanks for the feedback, I’ll > >> look at it some more. > > > > Thanks. ENOMEM also feels inappropriate as no allocation is taking > > place. Perhaps ENAMETOOLONG, which is used in similar cases for things > > like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh. > > > > Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything better. > > > Also, if pathlen has already been checked against SSIZE_MAX (giving > > EINVAL) and tmplen against pathlen there's no need to then check tmplen > > against SSIZE_MAX. > > > > Done. > > > I'd be happy to give a review on Phabricator if/when you have a new > > patch. > > > > https://reviews.freebsd.org/D27227 > > Thanks, > Scott > > -- Brandon Bergren bdra...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
> On Nov 15, 2020, at 12:01 PM, Jessica Clarke wrote: >> >> I felt similar concerns, but my misunderstanding of strlcpy() drove the >> result. Since the use case for getlocalbase() lends itself to also use >> strlcat()/strlcpy(), I was trying to replicate the API semantics of those, >> at least to the limit of my understanding. Thanks for the feedback, I’ll >> look at it some more. > > Thanks. ENOMEM also feels inappropriate as no allocation is taking > place. Perhaps ENAMETOOLONG, which is used in similar cases for things > like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh. > Yep, I wasn’t happy with ENOMEM either but I couldn’t find anything better. > Also, if pathlen has already been checked against SSIZE_MAX (giving > EINVAL) and tmplen against pathlen there's no need to then check tmplen > against SSIZE_MAX. > Done. > I'd be happy to give a review on Phabricator if/when you have a new > patch. > https://reviews.freebsd.org/D27227 Thanks, Scott ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
On 15 Nov 2020, at 18:46, Scott Long wrote:
>> On Nov 15, 2020, at 11:31 AM, Jessica Clarke wrote:
>>
>> Hi Scott,
>> I'm concerned by this diff; see my comments below.
>>
>>> On 15 Nov 2020, at 07:48, Scott Long wrote:
>>>
>>> Author: scottl
>>> Date: Sun Nov 15 07:48:52 2020
>>> New Revision: 367701
>>> URL: https://svnweb.freebsd.org/changeset/base/367701
>>>
>>> Log:
>>> Because getlocalbase() returns -1 on error, it needs to use a signed type
>>> internally. Do that, and make sure that conversations between signed and
>>> unsigned don't overflow
>>>
>>> Modified:
>>> head/lib/libutil/getlocalbase.c
>>>
>>> Modified: head/lib/libutil/getlocalbase.c
>>> ==
>>> --- head/lib/libutil/getlocalbase.c Sun Nov 15 01:54:44 2020
>>> (r367700)
>>> +++ head/lib/libutil/getlocalbase.c Sun Nov 15 07:48:52 2020
>>> (r367701)
>>> @@ -41,7 +41,7 @@ __FBSDID("$FreeBSD$");
>>> ssize_t
>>> getlocalbase(char *path, size_t pathlen)
>>> {
>>> - size_t tmplen;
>>> + ssize_t tmplen;
>>> const char *tmppath;
>>>
>>> if ((pathlen == 0) || (path == NULL)) {
>>> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen)
>>> return (-1);
>>> }
>>>
>>> + /* It's unlikely that the buffer would be this big */
>>> + if (pathlen > SSIZE_MAX) {
>>> + errno = ENOMEM;
>>> + return (-1);
>>> + }
>>> +
>>> tmppath = NULL;
>>> - tmplen = pathlen;
>>> + tmplen = (size_t)pathlen;
>>> if (issetugid() == 0)
>>> tmppath = getenv("LOCALBASE");
>>>
>>> if ((tmppath == NULL) &&
>>> - (sysctlbyname("user.localbase", path, , NULL, 0) == 0)) {
>>> + (sysctlbyname("user.localbase", path, (size_t *), NULL,
>>
>> This is dangerous and generally a sign of something wrong.
>
> I think that the danger was mitigated by the overflow check above, but I
> agree that this is generally a poor pattern.
>
>>
>>> + 0) == 0)) {
>>> return (tmplen);
>>> }
>>>
>>> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen)
>>> #endif
>>>
>>> tmplen = strlcpy(path, tmppath, pathlen);
>>> - if ((tmplen < 0) || (tmplen >= pathlen)) {
>>> + if ((tmplen < 0) || (tmplen >= (ssize_t)pathlen)) {
>>
>> As I commented on the previous commit (but which you do not appear to
>> have picked up on), the LHS is impossible. Of course, now the types
>> have changed so the compiler doesn't know that.
>>
>
> The man page for strlcpy() made reference to the return value being
> equivalent to what snprintf() does. The man page for snprintf() states
> that negatve return values are possible, so I assumed the same was
> true for strlcpy(). However, now that I’ve looked at the implementation
> of strlcpy(), I see that you’re correct. The man pages are definitely
> confusing, and this isn’t the only place where I think there’s
> inconsistency in the documentation, or at least poor wording choices.
>
>> I think tmplen should have remained a size_t, as everywhere it's used
>> you're having to cast, which is generally a sign you've done something
>> wrong. Only when you come to return from the function do you need a
>> single cast to ssize_t (provided you've checked for overflow first). I
>> strongly believe this entire commit should be reverted, and whatever
>> bug you were trying to fixed be fixed in another way without using the
>> wrong types and adding an array of unnecessary and/or dangerous casts.
>>
>
> I felt similar concerns, but my misunderstanding of strlcpy() drove the
> result. Since the use case for getlocalbase() lends itself to also use
> strlcat()/strlcpy(), I was trying to replicate the API semantics of those,
> at least to the limit of my understanding. Thanks for the feedback, I’ll
> look at it some more.
Thanks. ENOMEM also feels inappropriate as no allocation is taking
place. Perhaps ENAMETOOLONG, which is used in similar cases for things
like gethostbyname? Though sysctlbyname uses ENOMEM instead... sigh.
Also, if pathlen has already been checked against SSIZE_MAX (giving
EINVAL) and tmplen against pathlen there's no need to then check tmplen
against SSIZE_MAX.
I'd be happy to give a review on Phabricator if/when you have a new
patch.
Jess
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
> On Nov 15, 2020, at 11:31 AM, Jessica Clarke wrote:
>
> Hi Scott,
> I'm concerned by this diff; see my comments below.
>
>> On 15 Nov 2020, at 07:48, Scott Long wrote:
>>
>> Author: scottl
>> Date: Sun Nov 15 07:48:52 2020
>> New Revision: 367701
>> URL: https://svnweb.freebsd.org/changeset/base/367701
>>
>> Log:
>> Because getlocalbase() returns -1 on error, it needs to use a signed type
>> internally. Do that, and make sure that conversations between signed and
>> unsigned don't overflow
>>
>> Modified:
>> head/lib/libutil/getlocalbase.c
>>
>> Modified: head/lib/libutil/getlocalbase.c
>> ==
>> --- head/lib/libutil/getlocalbase.c Sun Nov 15 01:54:44 2020
>> (r367700)
>> +++ head/lib/libutil/getlocalbase.c Sun Nov 15 07:48:52 2020
>> (r367701)
>> @@ -41,7 +41,7 @@ __FBSDID("$FreeBSD$");
>> ssize_t
>> getlocalbase(char *path, size_t pathlen)
>> {
>> -size_t tmplen;
>> +ssize_t tmplen;
>> const char *tmppath;
>>
>> if ((pathlen == 0) || (path == NULL)) {
>> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen)
>> return (-1);
>> }
>>
>> +/* It's unlikely that the buffer would be this big */
>> +if (pathlen > SSIZE_MAX) {
>> +errno = ENOMEM;
>> +return (-1);
>> +}
>> +
>> tmppath = NULL;
>> -tmplen = pathlen;
>> +tmplen = (size_t)pathlen;
>> if (issetugid() == 0)
>> tmppath = getenv("LOCALBASE");
>>
>> if ((tmppath == NULL) &&
>> -(sysctlbyname("user.localbase", path, , NULL, 0) == 0)) {
>> +(sysctlbyname("user.localbase", path, (size_t *), NULL,
>
> This is dangerous and generally a sign of something wrong.
I think that the danger was mitigated by the overflow check above, but I
agree that this is generally a poor pattern.
>
>> +0) == 0)) {
>> return (tmplen);
>> }
>>
>> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen)
>> #endif
>>
>> tmplen = strlcpy(path, tmppath, pathlen);
>> -if ((tmplen < 0) || (tmplen >= pathlen)) {
>> +if ((tmplen < 0) || (tmplen >= (ssize_t)pathlen)) {
>
> As I commented on the previous commit (but which you do not appear to
> have picked up on), the LHS is impossible. Of course, now the types
> have changed so the compiler doesn't know that.
>
The man page for strlcpy() made reference to the return value being
equivalent to what snprintf() does. The man page for snprintf() states
that negatve return values are possible, so I assumed the same was
true for strlcpy(). However, now that I’ve looked at the implementation
of strlcpy(), I see that you’re correct. The man pages are definitely
confusing, and this isn’t the only place where I think there’s
inconsistency in the documentation, or at least poor wording choices.
> I think tmplen should have remained a size_t, as everywhere it's used
> you're having to cast, which is generally a sign you've done something
> wrong. Only when you come to return from the function do you need a
> single cast to ssize_t (provided you've checked for overflow first). I
> strongly believe this entire commit should be reverted, and whatever
> bug you were trying to fixed be fixed in another way without using the
> wrong types and adding an array of unnecessary and/or dangerous casts.
>
I felt similar concerns, but my misunderstanding of strlcpy() drove the
result. Since the use case for getlocalbase() lends itself to also use
strlcat()/strlcpy(), I was trying to replicate the API semantics of those,
at least to the limit of my understanding. Thanks for the feedback, I’ll
look at it some more.
Scott
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
Hi Scott,
I'm concerned by this diff; see my comments below.
> On 15 Nov 2020, at 07:48, Scott Long wrote:
>
> Author: scottl
> Date: Sun Nov 15 07:48:52 2020
> New Revision: 367701
> URL: https://svnweb.freebsd.org/changeset/base/367701
>
> Log:
> Because getlocalbase() returns -1 on error, it needs to use a signed type
> internally. Do that, and make sure that conversations between signed and
> unsigned don't overflow
>
> Modified:
> head/lib/libutil/getlocalbase.c
>
> Modified: head/lib/libutil/getlocalbase.c
> ==
> --- head/lib/libutil/getlocalbase.c Sun Nov 15 01:54:44 2020
> (r367700)
> +++ head/lib/libutil/getlocalbase.c Sun Nov 15 07:48:52 2020
> (r367701)
> @@ -41,7 +41,7 @@ __FBSDID("$FreeBSD$");
> ssize_t
> getlocalbase(char *path, size_t pathlen)
> {
> - size_t tmplen;
> + ssize_t tmplen;
> const char *tmppath;
>
> if ((pathlen == 0) || (path == NULL)) {
> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen)
> return (-1);
> }
>
> + /* It's unlikely that the buffer would be this big */
> + if (pathlen > SSIZE_MAX) {
> + errno = ENOMEM;
> + return (-1);
> + }
> +
> tmppath = NULL;
> - tmplen = pathlen;
> + tmplen = (size_t)pathlen;
> if (issetugid() == 0)
> tmppath = getenv("LOCALBASE");
>
> if ((tmppath == NULL) &&
> - (sysctlbyname("user.localbase", path, , NULL, 0) == 0)) {
> + (sysctlbyname("user.localbase", path, (size_t *), NULL,
This is dangerous and generally a sign of something wrong.
> + 0) == 0)) {
> return (tmplen);
> }
>
> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen)
> #endif
>
> tmplen = strlcpy(path, tmppath, pathlen);
> - if ((tmplen < 0) || (tmplen >= pathlen)) {
> + if ((tmplen < 0) || (tmplen >= (ssize_t)pathlen)) {
As I commented on the previous commit (but which you do not appear to
have picked up on), the LHS is impossible. Of course, now the types
have changed so the compiler doesn't know that.
I think tmplen should have remained a size_t, as everywhere it's used
you're having to cast, which is generally a sign you've done something
wrong. Only when you come to return from the function do you need a
single cast to ssize_t (provided you've checked for overflow first). I
strongly believe this entire commit should be reverted, and whatever
bug you were trying to fixed be fixed in another way without using the
wrong types and adding an array of unnecessary and/or dangerous casts.
Jess
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r367701 - head/lib/libutil
> Modified: head/lib/libutil/getlocalbase.c
> ==
> --- head/lib/libutil/getlocalbase.c Sun Nov 15 01:54:44 2020
> (r367700)
> +++ head/lib/libutil/getlocalbase.c Sun Nov 15 07:48:52 2020
> (r367701)
> @@ -41,7 +41,7 @@ __FBSDID("$FreeBSD$");
> ssize_t
> getlocalbase(char *path, size_t pathlen)
> {
> - size_t tmplen;
> + ssize_t tmplen;
> const char *tmppath;
>
> if ((pathlen == 0) || (path == NULL)) {
> @@ -49,13 +49,20 @@ getlocalbase(char *path, size_t pathlen)
> return (-1);
> }
>
> + /* It's unlikely that the buffer would be this big */
> + if (pathlen > SSIZE_MAX) {
> + errno = ENOMEM;
> + return (-1);
> + }
> +
> tmppath = NULL;
> - tmplen = pathlen;
> + tmplen = (size_t)pathlen;
Typo? Shouldn’t pathlen be cast to ssize_t?
> if (issetugid() == 0)
> tmppath = getenv("LOCALBASE");
>
> if ((tmppath == NULL) &&
> - (sysctlbyname("user.localbase", path, , NULL, 0) == 0)) {
> + (sysctlbyname("user.localbase", path, (size_t *), NULL,
> + 0) == 0)) {
> return (tmplen);
> }
>
> @@ -67,13 +74,13 @@ getlocalbase(char *path, size_t pathlen)
> #endif
>
> tmplen = strlcpy(path, tmppath, pathlen);
> - if ((tmplen < 0) || (tmplen >= pathlen)) {
> + if ((tmplen < 0) || (tmplen >= (ssize_t)pathlen)) {
> errno = ENOMEM;
> return (-1);
> }
>
> /* It's unlikely that the buffer would be this big */
> - if (tmplen >= SSIZE_MAX) {
> + if (tmplen > SSIZE_MAX) {
> errno = ENOMEM;
> return (-1);
> }
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
