svn commit: r367714 - head/sys/kern
Author: mjg Date: Mon Nov 16 03:12:21 2020 New Revision: 367714 URL: https://svnweb.freebsd.org/changeset/base/367714 Log: select: call seltdfini on process and thread exit Since thread_zone is marked NOFREE the thread_fini callback is never executed, meaning memory allocated by seltdinit is never released. Adding the call to thread_dtor is not sufficient as exiting processes cache the main thread. Modified: head/sys/kern/kern_exit.c head/sys/kern/kern_thread.c Modified: head/sys/kern/kern_exit.c == --- head/sys/kern/kern_exit.c Mon Nov 16 03:09:18 2020(r367713) +++ head/sys/kern/kern_exit.c Mon Nov 16 03:12:21 2020(r367714) @@ -355,6 +355,7 @@ exit1(struct thread *td, int rval, int signo) PROC_UNLOCK(p); umtx_thread_exit(td); + seltdfini(td); /* * Reset any sigio structures pointing to us as a result of Modified: head/sys/kern/kern_thread.c == --- head/sys/kern/kern_thread.c Mon Nov 16 03:09:18 2020(r367713) +++ head/sys/kern/kern_thread.c Mon Nov 16 03:12:21 2020(r367714) @@ -329,6 +329,7 @@ thread_ctor(void *mem, int size, void *arg, int flags) audit_thread_alloc(td); #endif umtx_thread_alloc(td); + MPASS(td->td_sel == NULL); return (0); } @@ -369,6 +370,7 @@ thread_dtor(void *mem, int size, void *arg) osd_thread_exit(td); td_softdep_cleanup(td); MPASS(td->td_su == NULL); + seltdfini(td); } /* @@ -405,7 +407,7 @@ thread_fini(void *mem, int size) turnstile_free(td->td_turnstile); sleepq_free(td->td_sleepqueue); umtx_thread_fini(td); - seltdfini(td); + MPASS(td->td_sel == NULL); } /* ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r367713 - head/sys/kern
Author: mjg
Date: Mon Nov 16 03:09:18 2020
New Revision: 367713
URL: https://svnweb.freebsd.org/changeset/base/367713
Log:
select: replace reference counting with memory barriers in selfd
Refcounting was added to combat a race between selfdfree and doselwakup,
but it adds avoidable overhead.
selfdfree detects it can free the object by ->sf_si == NULL, thus we can
ensure that the condition only holds after all accesses are completed.
Modified:
head/sys/kern/sys_generic.c
Modified: head/sys/kern/sys_generic.c
==
--- head/sys/kern/sys_generic.c Sun Nov 15 22:49:28 2020(r367712)
+++ head/sys/kern/sys_generic.c Mon Nov 16 03:09:18 2020(r367713)
@@ -156,7 +156,6 @@ struct selfd {
struct mtx *sf_mtx;/* Pointer to selinfo mtx. */
struct seltd*sf_td; /* (k) owning seltd. */
void*sf_cookie; /* (k) fd or pollfd. */
- u_int sf_refs;
};
MALLOC_DEFINE(M_SELFD, "selfd", "selfd");
@@ -1704,16 +1703,17 @@ static void
selfdfree(struct seltd *stp, struct selfd *sfp)
{
STAILQ_REMOVE(>st_selq, sfp, selfd, sf_link);
- if (sfp->sf_si != NULL) {
+ /*
+* Paired with doselwakeup.
+*/
+ if (atomic_load_acq_ptr((uintptr_t *)>sf_si) != (uintptr_t)NULL) {
mtx_lock(sfp->sf_mtx);
if (sfp->sf_si != NULL) {
TAILQ_REMOVE(>sf_si->si_tdlist, sfp, sf_threads);
- refcount_release(>sf_refs);
}
mtx_unlock(sfp->sf_mtx);
}
- if (refcount_release(>sf_refs))
- free(sfp, M_SELFD);
+ free(sfp, M_SELFD);
}
/* Drain the waiters tied to all the selfd belonging the specified selinfo. */
@@ -1766,7 +1766,6 @@ selrecord(struct thread *selector, struct selinfo *sip
*/
sfp->sf_si = sip;
sfp->sf_mtx = mtxp;
- refcount_init(>sf_refs, 2);
STAILQ_INSERT_TAIL(>st_selq, sfp, sf_link);
/*
* Now that we've locked the sip, check for initialization.
@@ -1820,14 +1819,15 @@ doselwakeup(struct selinfo *sip, int pri)
* sf_si seltdclear will know to ignore this si.
*/
TAILQ_REMOVE(>si_tdlist, sfp, sf_threads);
- sfp->sf_si = NULL;
stp = sfp->sf_td;
+ /*
+* Paired with selfdfree.
+*/
+ atomic_store_rel_ptr((uintptr_t *)>sf_si, (uintptr_t)NULL);
mtx_lock(>st_mtx);
stp->st_flags |= SELTD_PENDING;
cv_broadcastpri(>st_wait, pri);
mtx_unlock(>st_mtx);
- if (refcount_release(>sf_refs))
- free(sfp, M_SELFD);
}
mtx_unlock(sip->si_mtx);
}
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r367712 - head/tools/build/mk
Author: dim
Date: Sun Nov 15 22:49:28 2020
New Revision: 367712
URL: https://svnweb.freebsd.org/changeset/base/367712
Log:
Ensure make delete-old does not unlink the llvm-cxxfilt and its manpage,
after r367304 and r367324, when WITH_LLVM_CXXFILT is enabled.
Noticed by: "Herbert J. Skuhra"
MFC after:3 days
X-MFC-With: r367304
Modified:
head/tools/build/mk/OptionalObsoleteFiles.inc
Modified: head/tools/build/mk/OptionalObsoleteFiles.inc
==
--- head/tools/build/mk/OptionalObsoleteFiles.inc Sun Nov 15 20:24:59
2020(r367711)
+++ head/tools/build/mk/OptionalObsoleteFiles.inc Sun Nov 15 22:49:28
2020(r367712)
@@ -1538,7 +1538,6 @@ OLD_FILES+=usr/bin/lli
OLD_FILES+=usr/bin/llvm-as
OLD_FILES+=usr/bin/llvm-bcanalyzer
OLD_FILES+=usr/bin/llvm-cxxdump
-OLD_FILES+=usr/bin/llvm-cxxfilt
OLD_FILES+=usr/bin/llvm-diff
OLD_FILES+=usr/bin/llvm-dis
OLD_FILES+=usr/bin/llvm-dwarfdump
@@ -1562,7 +1561,6 @@ OLD_FILES+=usr/share/man/man1/llc.1.gz
OLD_FILES+=usr/share/man/man1/lli.1.gz
OLD_FILES+=usr/share/man/man1/llvm-as.1.gz
OLD_FILES+=usr/share/man/man1/llvm-bcanalyzer.1.gz
-OLD_FILES+=usr/share/man/man1/llvm-cxxfilt.1.gz
OLD_FILES+=usr/share/man/man1/llvm-diff.1.gz
OLD_FILES+=usr/share/man/man1/llvm-dis.1.gz
OLD_FILES+=usr/share/man/man1/llvm-dwarfdump.1
@@ -1577,6 +1575,11 @@ OLD_FILES+=usr/share/man/man1/opt.1.gz
.if ${MK_CLANG_EXTRAS} == no && ${MK_CLANG_FORMAT} == no
OLD_FILES+=usr/bin/clang-format
+.endif
+
+.if ${MK_CLANG_EXTRAS} == no && ${MK_LLVM_CXXFILT} == no
+OLD_FILES+=usr/bin/llvm-cxxfilt
+OLD_FILES+=usr/share/man/man1/llvm-cxxfilt.1.gz
.endif
.if ${MK_CPP} == no
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r367711 - in head: lib/libutil sbin/nvmecontrol usr.sbin/mailwrapper usr.sbin/pkg
Author: scottl
Date: Sun Nov 15 20:24:59 2020
New Revision: 367711
URL: https://svnweb.freebsd.org/changeset/base/367711
Log:
Revert the whole getlocalbase() set of changes while a different design is
hashed out.
Deleted:
head/lib/libutil/getlocalbase.3
head/lib/libutil/getlocalbase.c
Modified:
head/lib/libutil/Makefile
head/lib/libutil/libutil.h
head/sbin/nvmecontrol/comnd.c
head/sbin/nvmecontrol/comnd.h
head/sbin/nvmecontrol/nvmecontrol.c
head/usr.sbin/mailwrapper/mailwrapper.c
head/usr.sbin/pkg/Makefile
head/usr.sbin/pkg/pkg.c
Modified: head/lib/libutil/Makefile
==
--- head/lib/libutil/Makefile Sun Nov 15 14:04:27 2020(r367710)
+++ head/lib/libutil/Makefile Sun Nov 15 20:24:59 2020(r367711)
@@ -12,8 +12,7 @@ PACKAGE= runtime
LIB= util
SHLIB_MAJOR= 9
-SRCS= _secure_path.c auth.c expand_number.c flopen.c fparseln.c \
- getlocalbase.c gr_util.c \
+SRCS= _secure_path.c auth.c expand_number.c flopen.c fparseln.c gr_util.c \
hexdump.c humanize_number.c kinfo_getfile.c \
kinfo_getallproc.c kinfo_getproc.c kinfo_getvmmap.c \
kinfo_getvmobject.c kld.c \
@@ -31,7 +30,7 @@ CFLAGS+= -DINET6
CFLAGS+= -I${.CURDIR} -I${SRCTOP}/lib/libc/gen/
-MAN+= expand_number.3 flopen.3 fparseln.3 getlocalbase.3 hexdump.3 \
+MAN+= expand_number.3 flopen.3 fparseln.3 hexdump.3 \
humanize_number.3 kinfo_getallproc.3 kinfo_getfile.3 \
kinfo_getproc.3 kinfo_getvmmap.3 kinfo_getvmobject.3 kld.3 \
login_auth.3 login_cap.3 \
Modified: head/lib/libutil/libutil.h
==
--- head/lib/libutil/libutil.h Sun Nov 15 14:04:27 2020(r367710)
+++ head/lib/libutil/libutil.h Sun Nov 15 20:24:59 2020(r367711)
@@ -65,11 +65,6 @@ typedef __size_tsize_t;
#define_SIZE_T_DECLARED
#endif
-#ifndef _SSIZE_T_DECLARED
-typedef __ssize_t ssize_t;
-#define _SSIZE_T_DECLARED
-#endif
-
#ifndef _UID_T_DECLARED
typedef__uid_t uid_t;
#define_UID_T_DECLARED
@@ -103,7 +98,6 @@ int flopen(const char *_path, int _flags, ...);
intflopenat(int _dirfd, const char *_path, int _flags, ...);
intforkpty(int *_amaster, char *_name,
struct termios *_termp, struct winsize *_winp);
-ssize_tgetlocalbase(char *path, size_t pathlen);
void hexdump(const void *_ptr, int _length, const char *_hdr, int _flags);
inthumanize_number(char *_buf, size_t _len, int64_t _number,
const char *_suffix, int _scale, int _flags);
Modified: head/sbin/nvmecontrol/comnd.c
==
--- head/sbin/nvmecontrol/comnd.c Sun Nov 15 14:04:27 2020
(r367710)
+++ head/sbin/nvmecontrol/comnd.c Sun Nov 15 20:24:59 2020
(r367711)
@@ -287,7 +287,7 @@ bad_arg:
* Loads all the .so's from the specified directory.
*/
void
-cmd_load_dir(char *dir, cmd_load_cb_t cb, void *argp)
+cmd_load_dir(const char *dir __unused, cmd_load_cb_t cb __unused, void *argp
__unused)
{
DIR *d;
struct dirent *dent;
Modified: head/sbin/nvmecontrol/comnd.h
==
--- head/sbin/nvmecontrol/comnd.h Sun Nov 15 14:04:27 2020
(r367710)
+++ head/sbin/nvmecontrol/comnd.h Sun Nov 15 20:24:59 2020
(r367711)
@@ -96,7 +96,7 @@ void cmd_register(struct cmd *, struct cmd *);
int arg_parse(int argc, char * const *argv, const struct cmd *f);
void arg_help(int argc, char * const *argv, const struct cmd *f);
void cmd_init(void);
-void cmd_load_dir(char *dir, cmd_load_cb_t *cb, void *argp);
+void cmd_load_dir(const char *dir, cmd_load_cb_t *cb, void *argp);
int cmd_dispatch(int argc, char *argv[], const struct cmd *);
#endif /* COMND_H */
Modified: head/sbin/nvmecontrol/nvmecontrol.c
==
--- head/sbin/nvmecontrol/nvmecontrol.c Sun Nov 15 14:04:27 2020
(r367710)
+++ head/sbin/nvmecontrol/nvmecontrol.c Sun Nov 15 20:24:59 2020
(r367711)
@@ -38,7 +38,6 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
-#include
#include
#include
#include
@@ -179,17 +178,11 @@ get_nsid(int fd, char **ctrlr_str, uint32_t *nsid)
int
main(int argc, char *argv[])
{
- char locallib[MAXPATHLEN];
- size_t len;
cmd_init();
- snprintf(locallib, MAXPATHLEN, "/lib/nvmecontrol");
- cmd_load_dir(locallib, NULL, NULL);
- if ((len = getlocalbase(locallib, MAXPATHLEN)) > 0) {
- strlcat(locallib, "/lib/nvmecontrol", MAXPATHLEN);
- cmd_load_dir(locallib, NULL, NULL);
- }
+ cmd_load_dir("/lib/nvmecontrol", NULL, NULL);
+ cmd_load_dir(_PATH_LOCALBASE "/lib/nvmecontrol", NULL, NULL);
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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r367687 - in head: sbin/nvmecontrol usr.sbin/mailwrapper usr.sbin/pkg
I fixed this in r367702, sorry for the breakage.
Scott
> On Nov 15, 2020, at 11:12 AM, Brandon Bergren wrote:
>
>> --- head/usr.sbin/pkg/pkg.c Sat Nov 14 17:57:50 2020(r367686)
>> +++ head/usr.sbin/pkg/pkg.c Sat Nov 14 18:01:14 2020(r367687)
>> @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$");
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -1037,6 +1038,7 @@ main(int argc, char *argv[])
>> {
>> char pkgpath[MAXPATHLEN];
>> const char *pkgarg;
>> +size_t len;
>> int i;
>> bool bootstrap_only, force, yes;
>>
>> @@ -1045,8 +1047,11 @@ main(int argc, char *argv[])
>> pkgarg = NULL;
>> yes = false;
>>
>> -snprintf(pkgpath, MAXPATHLEN, "%s/sbin/pkg",
>> -getenv("LOCALBASE") ? getenv("LOCALBASE") : _PATH_LOCALBASE);
>> +if ((len = getlocalbase(pkgpath, MAXPATHLEN)) != 0) {
>> +fprintf(stderr, "Cannot determine local path\n");
>> +exit(EXIT_FAILURE);
>> +}
>
> This logic is broken, it is failing on kernels that DO have user.localbase.
>
>> +strlcat(pkgpath, "/sbin/pkg", MAXPATHLEN - len);
>>
>> if (argc > 1 && strcmp(argv[1], "bootstrap") == 0) {
>> bootstrap_only = true;
>>
>
> --
> Brandon Bergren
> bdra...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r367687 - in head: sbin/nvmecontrol usr.sbin/mailwrapper usr.sbin/pkg
> --- head/usr.sbin/pkg/pkg.c Sat Nov 14 17:57:50 2020(r367686)
> +++ head/usr.sbin/pkg/pkg.c Sat Nov 14 18:01:14 2020(r367687)
> @@ -43,6 +43,7 @@ __FBSDID("$FreeBSD$");
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1037,6 +1038,7 @@ main(int argc, char *argv[])
> {
> char pkgpath[MAXPATHLEN];
> const char *pkgarg;
> + size_t len;
> int i;
> bool bootstrap_only, force, yes;
>
> @@ -1045,8 +1047,11 @@ main(int argc, char *argv[])
> pkgarg = NULL;
> yes = false;
>
> - snprintf(pkgpath, MAXPATHLEN, "%s/sbin/pkg",
> - getenv("LOCALBASE") ? getenv("LOCALBASE") : _PATH_LOCALBASE);
> + if ((len = getlocalbase(pkgpath, MAXPATHLEN)) != 0) {
> + fprintf(stderr, "Cannot determine local path\n");
> + exit(EXIT_FAILURE);
> + }
This logic is broken, it is failing on kernels that DO have user.localbase.
> + strlcat(pkgpath, "/sbin/pkg", MAXPATHLEN - len);
>
> if (argc > 1 && strcmp(argv[1], "bootstrap") == 0) {
> bootstrap_only = true;
>
--
Brandon Bergren
bdra...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r367433 - in head/sys: compat/linux conf
On Fri, 6 Nov 2020 22:04:57 + (UTC) Conrad Meyer
wrote:
> Author: cem
> Date: Fri Nov 6 22:04:57 2020
> New Revision: 367433
> URL: https://svnweb.freebsd.org/changeset/base/367433
>
> Log:
> linux(4): Fix loadable modules after r367395
>
> Move dtrace SDT definitions into linux_common module code. Also, build
> linux_dummy.c into the linux_common kld -- we don't need separate
> versions of these stubs for 32- and 64-bit emulation.
>
> Reported by:several
> PR: 250897
> Discussed with: emaste, trasz
> Tested by: John Kennedy, Yasuhiro KIMURA, Oleg Sidorkin
> X-MFC-With: r367395
> Differential Revision: https://reviews.freebsd.org/D27124
>
> Modified:
> head/sys/compat/linux/linux_common.c
> head/sys/compat/linux/linux_dummy.c
> head/sys/compat/linux/linux_misc.c
> head/sys/conf/files.i386
>
> Modified: head/sys/compat/linux/linux_common.c
> ==
> --- head/sys/compat/linux/linux_common.c Fri Nov 6 21:33:59 2020
> (r367432)
> +++ head/sys/compat/linux/linux_common.c Fri Nov 6 22:04:57 2020
> (r367433)
> @@ -39,6 +39,7 @@ __FBSDID("$FreeBSD$");
> #include
>
> #include
> +#include
> #include
> #include
> #include
> @@ -48,6 +49,20 @@ FEATURE(linuxulator_v4l, "V4L ioctl wrapper support in
> FEATURE(linuxulator_v4l2, "V4L2 ioctl wrapper support in the linuxulator");
>
> MODULE_VERSION(linux_common, 1);
> +
> +/**
> + * Special DTrace provider for the linuxulator.
> + *
> + * In this file we define the provider for the entire linuxulator. All
> + * modules (= files of the linuxulator) use it.
> + *
> + * We define a different name depending on the emulated bitsize, see
> + * ../..//linux{,32}/linux.h, e.g.:
> + * native bitsize = linuxulator
> + * amd64, 32bit emulation = linuxulator32
> + */
> +LIN_SDT_PROVIDER_DEFINE(linuxulator);
> +LIN_SDT_PROVIDER_DEFINE(linuxulator32);
>
> SET_DECLARE(linux_device_handler_set, struct linux_device_handler);
>
>
> Modified: head/sys/compat/linux/linux_dummy.c
> ==
> --- head/sys/compat/linux/linux_dummy.c Fri Nov 6 21:33:59 2020
> (r367432)
> +++ head/sys/compat/linux/linux_dummy.c Fri Nov 6 22:04:57 2020
> (r367433)
> @@ -29,21 +29,19 @@
> #include
> __FBSDID("$FreeBSD$");
>
> -#include "opt_compat.h"
> -
> #include
> #include
> #include
> #include
> #include
>
> -#ifdef COMPAT_LINUX32
> -#include
> -#include
> -#else
> +/*
> + * Including linux vs linux32 here is arbitrary -- the syscall args
> structures
> + * (proto.h) are not dereferenced by the DUMMY stub implementations, and
> + * suitable for use by both native and compat32 entrypoints.
> + */
> #include
> #include
> -#endif
>
> #include
> #include
>
> Modified: head/sys/compat/linux/linux_misc.c
> ==
> --- head/sys/compat/linux/linux_misc.cFri Nov 6 21:33:59 2020
> (r367432)
> +++ head/sys/compat/linux/linux_misc.cFri Nov 6 22:04:57 2020
> (r367433)
> @@ -99,19 +99,6 @@ __FBSDID("$FreeBSD$");
> #include
> #include
>
> -/**
> - * Special DTrace provider for the linuxulator.
> - *
> - * In this file we define the provider for the entire linuxulator. All
> - * modules (= files of the linuxulator) use it.
> - *
> - * We define a different name depending on the emulated bitsize, see
> - * ../..//linux{,32}/linux.h, e.g.:
> - * native bitsize = linuxulator
> - * amd64, 32bit emulation = linuxulator32
> - */
> -LIN_SDT_PROVIDER_DEFINE(LINUX_DTRACE);
> -
> int stclohz; /* Statistics clock frequency */
>
> static unsigned int linux_to_bsd_resource[LINUX_RLIM_NLIMITS] = {
>
> Modified: head/sys/conf/files.i386
> ==
> --- head/sys/conf/files.i386 Fri Nov 6 21:33:59 2020(r367432)
> +++ head/sys/conf/files.i386 Fri Nov 6 22:04:57 2020(r367433)
> @@ -52,6 +52,7 @@ cddl/dev/dtrace/i386/dtrace_asm.S
> optional dtrace co
> cddl/dev/dtrace/i386/dtrace_subr.c optional dtrace
> compile-with "${DTRACE_C}"
> compat/linprocfs/linprocfs.c optional linprocfs
> compat/linsysfs/linsysfs.c optional linsysfs
> +compat/linux/linux_common.c optional compat_linux
> compat/linux/linux_dummy.c optional compat_linux
> compat/linux/linux_event.c optional compat_linux
> compat/linux/linux_emul.coptional compat_linux
linux_common.c defines the linux_common module which contains stuff that
is common between 64 bit and 32 bit linux on amd64, but there's no such
module on i386, which is why the file wasn't in files.i386. You should
put LIN_SDT_PROVIDER_DEFINE in a file that is in
svn commit: r367710 - head/stand/i386/zfsboot
Author: tsoome
Date: Sun Nov 15 14:04:27 2020
New Revision: 367710
URL: https://svnweb.freebsd.org/changeset/base/367710
Log:
zfsboot: add prototype for main()
Some compilers are complaining about missing prototype.
PR: 251150
Reported by: markiyan.kush...@gmail.com
Modified:
head/stand/i386/zfsboot/zfsboot.c
Modified: head/stand/i386/zfsboot/zfsboot.c
==
--- head/stand/i386/zfsboot/zfsboot.c Sun Nov 15 12:59:24 2020
(r367709)
+++ head/stand/i386/zfsboot/zfsboot.c Sun Nov 15 14:04:27 2020
(r367710)
@@ -161,6 +161,8 @@ ptov(uintptr_t x)
return (PTOV(x));
}
+int main(void);
+
int
main(void)
{
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r367709 - head/usr.sbin/bhyve
Author: grehan
Date: Sun Nov 15 12:59:24 2020
New Revision: 367709
URL: https://svnweb.freebsd.org/changeset/base/367709
Log:
Fix regression in AHCI controller settings.
When the AHCI code was reworked to use FreeBSD struct
definitions, the valid element was mis-transcribed resulting
in the UMDA capability being hidden. This prevented Illumos
from using AHCI disk/cdrom drives.
Fix by using definitions that match the code pre-rework.
PR: 250924
Submitted by: Rolf Stalder
Reported by: Rolf Stalder
MFC after:3 days
Modified:
head/usr.sbin/bhyve/pci_ahci.c
Modified: head/usr.sbin/bhyve/pci_ahci.c
==
--- head/usr.sbin/bhyve/pci_ahci.c Sun Nov 15 12:31:07 2020
(r367708)
+++ head/usr.sbin/bhyve/pci_ahci.c Sun Nov 15 12:59:24 2020
(r367709)
@@ -1004,7 +1004,7 @@ ata_identify_init(struct ahci_port* p, int atapi)
ata_ident->capabilities1 = ATA_SUPPORT_LBA |
ATA_SUPPORT_DMA;
ata_ident->capabilities2 = (1 << 14 | 1);
- ata_ident->atavalid = ATA_FLAG_54_58 | ATA_FLAG_64_70;
+ ata_ident->atavalid = ATA_FLAG_64_70 | ATA_FLAG_88;
ata_ident->obsolete62 = 0x3f;
ata_ident->mwdmamodes = 7;
if (p->xfermode & ATA_WDMA0)
@@ -1053,8 +1053,7 @@ ata_identify_init(struct ahci_port* p, int atapi)
ata_ident->capabilities1 = ATA_SUPPORT_DMA |
ATA_SUPPORT_LBA | ATA_SUPPORT_IORDY;
ata_ident->capabilities2 = (1 << 14);
- ata_ident->atavalid = ATA_FLAG_54_58 |
- ATA_FLAG_64_70;
+ ata_ident->atavalid = ATA_FLAG_64_70 | ATA_FLAG_88;
if (p->mult_sectors)
ata_ident->multi = (ATA_MULTI_VALID | p->mult_sectors);
if (sectors <= 0x0fff) {
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-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-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
