Re: CVS commit: src/lib/libpthread
Good idea. It could be checked quicker... however I presume that t1->pt_magic + t1->pt_magic already crash on invalid t1/t2 pointers as the argument with condition is evaluated. Ryo, you might check: $ export PTHREAD_DIAGASSERT=ae $ firefox It should create a coredump for investigation. According to POSIX (https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_equal.html) passing invalid parameters is UB. GLIBC, Illumos and all other BSDs (+ older NetBSD) have no sanity check in pthread_equal(3). Apparently we are the first ones to notice the bug. On 01.02.2020 21:18, Andrew Doran wrote: > Hmm. Was there not originally an environment variable to control this > behaviour, since many applications are buggy? > > Andrew > > On Sun, Feb 02, 2020 at 01:01:49AM +0900, Ryo ONODERA wrote: >> Hi, >> >> pthread__error()s in pthread_equal() cause segfault >> during start of pkgsrc/www/firefox-72.0.2. >> >> Without pthread__error()s, www/firefox works fine >> like as follows. >> However I have no idea why I get segfaults. >> >> Could you take a look at this problem? >> >> Index: lib/libpthread/pthread.c >> === >> RCS file: /cvsroot/src/lib/libpthread/pthread.c,v >> retrieving revision 1.162 >> diff -u -r1.162 pthread.c >> --- lib/libpthread/pthread.c 29 Jan 2020 17:11:57 - 1.162 >> +++ lib/libpthread/pthread.c 1 Feb 2020 15:58:03 - >> @@ -770,11 +770,13 @@ >> if (__predict_false(__uselibcstub)) >> return __libc_thr_equal_stub(t1, t2); >> >> +#if 0 >> pthread__error(EINVAL, "Invalid thread", >> t1->pt_magic == PT_MAGIC); >> >> pthread__error(EINVAL, "Invalid thread", >> t2->pt_magic == PT_MAGIC); >> +#endif >> >> /* Nothing special here. */ >> return (t1 == t2); >> @@ -1108,7 +1110,7 @@ >> { >> char buf[1024]; >> size_t len; >> - >> + >> if (pthread__diagassert == 0) >> return; >> >> >> >> "Kamil Rytarowski" writes: >> >>> Module Name:src >>> Committed By: kamil >>> Date: Wed Jan 29 16:03:44 UTC 2020 >>> >>> Modified Files: >>> src/lib/libpthread: pthread.c pthread_getcpuclockid.c >>> >>> Log Message: >>> Chack thread->pt_magic with PT_MAGIC promptly >>> >>> Rearrange some checks to avoid verifying pthread_t after using it. >>> >>> >>> To generate a diff of this commit: >>> cvs rdiff -u -r1.160 -r1.161 src/lib/libpthread/pthread.c >>> cvs rdiff -u -r1.2 -r1.3 src/lib/libpthread/pthread_getcpuclockid.c >>> >>> Please note that diffs are not public domain; they are subject to the >>> copyright notices on the relevant files. >>> >> >> -- >> Ryo ONODERA // r...@tetera.org >> PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3 signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/libpthread
Hmm. Was there not originally an environment variable to control this behaviour, since many applications are buggy? Andrew On Sun, Feb 02, 2020 at 01:01:49AM +0900, Ryo ONODERA wrote: > Hi, > > pthread__error()s in pthread_equal() cause segfault > during start of pkgsrc/www/firefox-72.0.2. > > Without pthread__error()s, www/firefox works fine > like as follows. > However I have no idea why I get segfaults. > > Could you take a look at this problem? > > Index: lib/libpthread/pthread.c > === > RCS file: /cvsroot/src/lib/libpthread/pthread.c,v > retrieving revision 1.162 > diff -u -r1.162 pthread.c > --- lib/libpthread/pthread.c 29 Jan 2020 17:11:57 - 1.162 > +++ lib/libpthread/pthread.c 1 Feb 2020 15:58:03 - > @@ -770,11 +770,13 @@ > if (__predict_false(__uselibcstub)) > return __libc_thr_equal_stub(t1, t2); > > +#if 0 > pthread__error(EINVAL, "Invalid thread", > t1->pt_magic == PT_MAGIC); > > pthread__error(EINVAL, "Invalid thread", > t2->pt_magic == PT_MAGIC); > +#endif > > /* Nothing special here. */ > return (t1 == t2); > @@ -1108,7 +1110,7 @@ > { > char buf[1024]; > size_t len; > - > + > if (pthread__diagassert == 0) > return; > > > > "Kamil Rytarowski" writes: > > > Module Name:src > > Committed By: kamil > > Date: Wed Jan 29 16:03:44 UTC 2020 > > > > Modified Files: > > src/lib/libpthread: pthread.c pthread_getcpuclockid.c > > > > Log Message: > > Chack thread->pt_magic with PT_MAGIC promptly > > > > Rearrange some checks to avoid verifying pthread_t after using it. > > > > > > To generate a diff of this commit: > > cvs rdiff -u -r1.160 -r1.161 src/lib/libpthread/pthread.c > > cvs rdiff -u -r1.2 -r1.3 src/lib/libpthread/pthread_getcpuclockid.c > > > > Please note that diffs are not public domain; they are subject to the > > copyright notices on the relevant files. > > > > -- > Ryo ONODERA // r...@tetera.org > PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/common/lib/libc/arch
On Sat, Feb 01, 2020 at 03:02:02PM +, m...@netbsd.org wrote: > On Mon, Jan 27, 2020 at 10:09:21PM +, Andrew Doran wrote: > > Module Name:src > > Committed By: ad > > Date: Mon Jan 27 22:09:21 UTC 2020 > > > > Removed Files: > > src/common/lib/libc/arch/i386/string: memcmp.S > > src/common/lib/libc/arch/x86_64/string: bcmp.S memcmp.S > > > > Log Message: > > x86 uses the C versions of bcmp() and memcmp() now. > > > > Why? REP CMPS is very slow on the modern CPUs I have access to. The updated C version is 1.5-10x faster in the configurations I have tried, from tiny strings up to N-megabyte strings. MOVS and STOS are very good though. Andrew
Re: CVS commit: src/sys
On Sat, Feb 01, 2020 at 02:23:24AM +, Taylor R Campbell wrote: > Module Name: src > Committed By: riastradh > Date: Sat Feb 1 02:23:23 UTC 2020 > > Modified Files: > src/sys/ddb: db_xxx.c > src/sys/kern: kern_descrip.c kern_sig.c subr_exec_fd.c uipc_socket2.c > uipc_usrreq.c > > Log Message: > Load struct fdfile::ff_file with atomic_load_consume. > > Exceptions: when we're only testing whether it's there, not about to > dereference it. > > Note: We do not use atomic_store_release to set it because the > preceding mutex_exit should be enough. > > (That said, it's not clear the mutex_enter/exit is needed unless > refcnt > 0 already, in which case maybe it would be a win to switch > from the membar implied by mutex_enter to the membar implied by > atomic_store_release -- which I would generally expect to be much > cheaper. And a little clearer without a long comment.) Likely procfs and sysctl since they go the strongly-locked route. Andrew
Re: CVS commit: src/lib/libpthread
Kamil Rytarowski writes: > On 01.02.2020 17:01, Ryo ONODERA wrote: >> Hi, >> >> pthread__error()s in pthread_equal() cause segfault >> during start of pkgsrc/www/firefox-72.0.2. >> >> Without pthread__error()s, www/firefox works fine >> like as follows. >> However I have no idea why I get segfaults. >> >> Could you take a look at this problem? >> >> Index: lib/libpthread/pthread.c >> === >> RCS file: /cvsroot/src/lib/libpthread/pthread.c,v >> retrieving revision 1.162 >> diff -u -r1.162 pthread.c >> --- lib/libpthread/pthread.c 29 Jan 2020 17:11:57 - 1.162 >> +++ lib/libpthread/pthread.c 1 Feb 2020 15:58:03 - >> @@ -770,11 +770,13 @@ >> if (__predict_false(__uselibcstub)) >> return __libc_thr_equal_stub(t1, t2); >> >> +#if 0 >> pthread__error(EINVAL, "Invalid thread", >> t1->pt_magic == PT_MAGIC); >> >> pthread__error(EINVAL, "Invalid thread", >> t2->pt_magic == PT_MAGIC); >> +#endif >> >> /* Nothing special here. */ >> return (t1 == t2); >> @@ -1108,7 +1110,7 @@ >> { >> char buf[1024]; >> size_t len; >> - >> + >> if (pthread__diagassert == 0) >> return; >> >> >> >> "Kamil Rytarowski" writes: >> >>> Module Name:src >>> Committed By: kamil >>> Date: Wed Jan 29 16:03:44 UTC 2020 >>> >>> Modified Files: >>> src/lib/libpthread: pthread.c pthread_getcpuclockid.c >>> >>> Log Message: >>> Chack thread->pt_magic with PT_MAGIC promptly >>> >>> Rearrange some checks to avoid verifying pthread_t after using it. >>> >>> >>> To generate a diff of this commit: >>> cvs rdiff -u -r1.160 -r1.161 src/lib/libpthread/pthread.c >>> cvs rdiff -u -r1.2 -r1.3 src/lib/libpthread/pthread_getcpuclockid.c >>> >>> Please note that diffs are not public domain; they are subject to the >>> copyright notices on the relevant files. >>> >> > > I will have a look, but it will take a while to build firefox for me. Thank you. Of course I can wait. -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/lib/libpthread
On 01.02.2020 17:01, Ryo ONODERA wrote: > Hi, > > pthread__error()s in pthread_equal() cause segfault > during start of pkgsrc/www/firefox-72.0.2. > > Without pthread__error()s, www/firefox works fine > like as follows. > However I have no idea why I get segfaults. > > Could you take a look at this problem? > > Index: lib/libpthread/pthread.c > === > RCS file: /cvsroot/src/lib/libpthread/pthread.c,v > retrieving revision 1.162 > diff -u -r1.162 pthread.c > --- lib/libpthread/pthread.c 29 Jan 2020 17:11:57 - 1.162 > +++ lib/libpthread/pthread.c 1 Feb 2020 15:58:03 - > @@ -770,11 +770,13 @@ > if (__predict_false(__uselibcstub)) > return __libc_thr_equal_stub(t1, t2); > > +#if 0 > pthread__error(EINVAL, "Invalid thread", > t1->pt_magic == PT_MAGIC); > > pthread__error(EINVAL, "Invalid thread", > t2->pt_magic == PT_MAGIC); > +#endif > > /* Nothing special here. */ > return (t1 == t2); > @@ -1108,7 +1110,7 @@ > { > char buf[1024]; > size_t len; > - > + > if (pthread__diagassert == 0) > return; > > > > "Kamil Rytarowski" writes: > >> Module Name: src >> Committed By:kamil >> Date:Wed Jan 29 16:03:44 UTC 2020 >> >> Modified Files: >> src/lib/libpthread: pthread.c pthread_getcpuclockid.c >> >> Log Message: >> Chack thread->pt_magic with PT_MAGIC promptly >> >> Rearrange some checks to avoid verifying pthread_t after using it. >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.160 -r1.161 src/lib/libpthread/pthread.c >> cvs rdiff -u -r1.2 -r1.3 src/lib/libpthread/pthread_getcpuclockid.c >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. >> > I will have a look, but it will take a while to build firefox for me. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/libpthread
Hi, pthread__error()s in pthread_equal() cause segfault during start of pkgsrc/www/firefox-72.0.2. Without pthread__error()s, www/firefox works fine like as follows. However I have no idea why I get segfaults. Could you take a look at this problem? Index: lib/libpthread/pthread.c === RCS file: /cvsroot/src/lib/libpthread/pthread.c,v retrieving revision 1.162 diff -u -r1.162 pthread.c --- lib/libpthread/pthread.c29 Jan 2020 17:11:57 - 1.162 +++ lib/libpthread/pthread.c1 Feb 2020 15:58:03 - @@ -770,11 +770,13 @@ if (__predict_false(__uselibcstub)) return __libc_thr_equal_stub(t1, t2); +#if 0 pthread__error(EINVAL, "Invalid thread", t1->pt_magic == PT_MAGIC); pthread__error(EINVAL, "Invalid thread", t2->pt_magic == PT_MAGIC); +#endif /* Nothing special here. */ return (t1 == t2); @@ -1108,7 +1110,7 @@ { char buf[1024]; size_t len; - + if (pthread__diagassert == 0) return; "Kamil Rytarowski" writes: > Module Name: src > Committed By: kamil > Date: Wed Jan 29 16:03:44 UTC 2020 > > Modified Files: > src/lib/libpthread: pthread.c pthread_getcpuclockid.c > > Log Message: > Chack thread->pt_magic with PT_MAGIC promptly > > Rearrange some checks to avoid verifying pthread_t after using it. > > > To generate a diff of this commit: > cvs rdiff -u -r1.160 -r1.161 src/lib/libpthread/pthread.c > cvs rdiff -u -r1.2 -r1.3 src/lib/libpthread/pthread_getcpuclockid.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > -- Ryo ONODERA // r...@tetera.org PGP fingerprint = 82A2 DC91 76E0 A10A 8ABB FD1B F404 27FA C7D1 15F3
Re: CVS commit: src/lib/libpthread
Actually it happened that modifiying pthread_atfork() to stop malloc()ing is enough to address the problem. I have landed the changes and removed '#if 0' kludge. Thanks! On 01.02.2020 13:59, Kamil Rytarowski wrote: > On 31.01.2020 22:10, Andrew Doran wrote: >> On Fri, Jan 31, 2020 at 06:55:00PM -, Christos Zoulas wrote: >> >>> In article <724af477-010b-9ddf-6ece-e23d7cf59...@gmx.com>, >>> Kamil Rytarowski wrote: -=-=-=-=-=- -=-=-=-=-=- On 31.01.2020 03:38, Christos Zoulas wrote: > And it is fixed now. > > christos > OK. I am going to submit a bug report upstream and get some feedback what is the way forward here, delaying initialization. >>> >>> I think that the way forward (on our side) is to do away with libpthread, >>> merge it with libc and kill all the stub nonsense. >> >> Agreed. >> >> pthread__init() does some expensive stuff like _lwp_ctl(). I think we can >> safely & without hacks defer a lot of that till the first pthread_create(). >> >> Andrew >> > > This libc-libpthread split/merge is a red herring. > > The problem here is with a mutual dependencies between POSIX threads > library and malloc library. > > I did some investigation and here are my findings: > > 1. jemalloc abuses initialization and initializes self very early, with > a constructor: > > /* > * If an application creates a thread before doing any allocation in the > main > * thread, then calls fork(2) in the main thread followed by memory > allocation > * in the child process, a race can occur that results in deadlock > within the > * child: the main thread may have forked while the created thread had > * partially initialized the allocator. Ordinarily jemalloc prevents > * fork/malloc races via the following functions it registers during > * initialization using pthread_atfork(), but of course that does no good if > * the allocator isn't fully initialized at fork time. The following > library > * constructor is a partial solution to this problem. It may still be > possible > * to trigger the deadlock described above, but doing so would involve > forking > * via a library constructor that runs before jemalloc's runs. > */ > #ifndef JEMALLOC_JET > JEMALLOC_ATTR(constructor) > static void > > jemalloc_constructor(void) { > malloc_init(); > } > #endif > > Relevant commit: > > commit 20f1fc95adb35ea63dc61f47f2b0ffbd37d39f32 > Author: Jason Evans > Date: Tue Oct 9 14:46:22 2012 -0700 > > Fix fork(2)-related deadlocks. > > Add a library constructor for jemalloc that initializes the allocator. > This fixes a race that could occur if threads were created by the main > thread prior to any memory allocation, followed by fork(2), and then > memory allocation in the child process. > > Fix the prefork/postfork functions to acquire/release the ctl, prof, and > rtree mutexes. This fixes various fork() child process deadlocks, but > one possible deadlock remains (intentionally) unaddressed: prof > backtracing can acquire runtime library mutexes, so deadlock is still > possible if heap profiling is enabled during fork(). This deadlock is > known to be a real issue in at least the case of libgcc-based > backtracing. > > Reported by tfengjun. > > 2. FreeBSD added a hack and an internal pthread_mutex_init() version > called: _pthread_mutex_init_calloc_cb().. it passes a callback pointer > to jemalloc's tiny calloc(). > > This is very ugly and I consider it as the wrong way of boostraping malloc. > > 3. There is a problem inside libpthread. It as designed to not malloc() > early to not trigger malloc initialization, however it was broken as it > calls at_fork functions: > > #0 malloc (size=size@entry=16) at > /usr/src/external/bsd/jemalloc/lib/../dist/src/jemalloc.c:2052 > #1 0x776b71d71a44 in af_alloc () at > /usr/src/lib/libc/gen/pthread_atfork.c:80 > #2 af_alloc () at /usr/src/lib/libc/gen/pthread_atfork.c:74 > #3 _pthread_atfork (prepare=prepare@entry=0x0, parent=parent@entry=0x0, > child=child@entry=0x776b7220b3e5 ) > at /usr/src/lib/libc/gen/pthread_atfork.c:121 > #4 0x776b7220cacd in pthread__init () at > /usr/src/lib/libpthread/pthread.c:260 > #5 0x776b71d7a585 in _libc_init () at > /usr/src/lib/libc/misc/initfini.c:128 > > These at_fork routines caused also issues with false-positives in Leak > Sanitizer. I had to pacify the sanitizer and disable tracking of its > allocations. > > > This patch removes '#if 0' hack from src/lib/libpthread and switches > at_fork to mmap()+munmap(). > > http://netbsd.org/~kamil/patch-00219-libpthread-libc-jemalloc.txt > > This test disabled the constructor hack: > > http://netbsd.org/~kamil/patch-00220-jemalloc-disable-constructor.txt > > With these changes everything seems to work. > > In order to avoid the FreeBSD specific hack with the constructor and > initialize jemalloc always during libc bootstrap I propose the following
Re: CVS commit: src/common/lib/libc/arch
On Mon, Jan 27, 2020 at 10:09:21PM +, Andrew Doran wrote: > Module Name: src > Committed By: ad > Date: Mon Jan 27 22:09:21 UTC 2020 > > Removed Files: > src/common/lib/libc/arch/i386/string: memcmp.S > src/common/lib/libc/arch/x86_64/string: bcmp.S memcmp.S > > Log Message: > x86 uses the C versions of bcmp() and memcmp() now. > Why?
Re: CVS commit: src/lib/libpthread
On 31.01.2020 22:10, Andrew Doran wrote: > On Fri, Jan 31, 2020 at 06:55:00PM -, Christos Zoulas wrote: > >> In article <724af477-010b-9ddf-6ece-e23d7cf59...@gmx.com>, >> Kamil Rytarowski wrote: >>> -=-=-=-=-=- >>> -=-=-=-=-=- >>> >>> On 31.01.2020 03:38, Christos Zoulas wrote: And it is fixed now. christos >>> >>> OK. I am going to submit a bug report upstream and get some feedback >>> what is the way forward here, delaying initialization. >> >> I think that the way forward (on our side) is to do away with libpthread, >> merge it with libc and kill all the stub nonsense. > > Agreed. > > pthread__init() does some expensive stuff like _lwp_ctl(). I think we can > safely & without hacks defer a lot of that till the first pthread_create(). > > Andrew > This libc-libpthread split/merge is a red herring. The problem here is with a mutual dependencies between POSIX threads library and malloc library. I did some investigation and here are my findings: 1. jemalloc abuses initialization and initializes self very early, with a constructor: /* * If an application creates a thread before doing any allocation in the main * thread, then calls fork(2) in the main thread followed by memory allocation * in the child process, a race can occur that results in deadlock within the * child: the main thread may have forked while the created thread had * partially initialized the allocator. Ordinarily jemalloc prevents * fork/malloc races via the following functions it registers during * initialization using pthread_atfork(), but of course that does no good if * the allocator isn't fully initialized at fork time. The following library * constructor is a partial solution to this problem. It may still be possible * to trigger the deadlock described above, but doing so would involve forking * via a library constructor that runs before jemalloc's runs. */ #ifndef JEMALLOC_JET JEMALLOC_ATTR(constructor) static void jemalloc_constructor(void) { malloc_init(); } #endif Relevant commit: commit 20f1fc95adb35ea63dc61f47f2b0ffbd37d39f32 Author: Jason Evans Date: Tue Oct 9 14:46:22 2012 -0700 Fix fork(2)-related deadlocks. Add a library constructor for jemalloc that initializes the allocator. This fixes a race that could occur if threads were created by the main thread prior to any memory allocation, followed by fork(2), and then memory allocation in the child process. Fix the prefork/postfork functions to acquire/release the ctl, prof, and rtree mutexes. This fixes various fork() child process deadlocks, but one possible deadlock remains (intentionally) unaddressed: prof backtracing can acquire runtime library mutexes, so deadlock is still possible if heap profiling is enabled during fork(). This deadlock is known to be a real issue in at least the case of libgcc-based backtracing. Reported by tfengjun. 2. FreeBSD added a hack and an internal pthread_mutex_init() version called: _pthread_mutex_init_calloc_cb().. it passes a callback pointer to jemalloc's tiny calloc(). This is very ugly and I consider it as the wrong way of boostraping malloc. 3. There is a problem inside libpthread. It as designed to not malloc() early to not trigger malloc initialization, however it was broken as it calls at_fork functions: #0 malloc (size=size@entry=16) at /usr/src/external/bsd/jemalloc/lib/../dist/src/jemalloc.c:2052 #1 0x776b71d71a44 in af_alloc () at /usr/src/lib/libc/gen/pthread_atfork.c:80 #2 af_alloc () at /usr/src/lib/libc/gen/pthread_atfork.c:74 #3 _pthread_atfork (prepare=prepare@entry=0x0, parent=parent@entry=0x0, child=child@entry=0x776b7220b3e5 ) at /usr/src/lib/libc/gen/pthread_atfork.c:121 #4 0x776b7220cacd in pthread__init () at /usr/src/lib/libpthread/pthread.c:260 #5 0x776b71d7a585 in _libc_init () at /usr/src/lib/libc/misc/initfini.c:128 These at_fork routines caused also issues with false-positives in Leak Sanitizer. I had to pacify the sanitizer and disable tracking of its allocations. This patch removes '#if 0' hack from src/lib/libpthread and switches at_fork to mmap()+munmap(). http://netbsd.org/~kamil/patch-00219-libpthread-libc-jemalloc.txt This test disabled the constructor hack: http://netbsd.org/~kamil/patch-00220-jemalloc-disable-constructor.txt With these changes everything seems to work. In order to avoid the FreeBSD specific hack with the constructor and initialize jemalloc always during libc bootstrap I propose the following approach: - add __libc_malloc_init() call in _libc_init() - redirect __libc_malloc_init() to jemalloc__init() with jemalloc - otherwise redirect to an empty stub Here is a patch that does everything and works fine for me. http://netbsd.org/~kamil/patch-00222-jemalloc-enhancements.txt There are no longer jemalloc calls before being ready and jemalloc is still initialized always but in its proper time. (gdb) r The program being debugged