[<prev] [next>] [day] [month] [year] [list]
Message-ID: <tencent_9A620609B3300E395885FA8CCCDDD5689A06@qq.com>
Date: Thu, 11 Jul 2024 22:40:22 +0800
From: "AK47" <250200715@...com>
To: "musl" <musl@...ts.openwall.com>
Subject: Re:RE: Maybe A Bug about timer_create and pthread_barrier_wait
Hello:<br/><br/>&gt;pthread_barrier_destroy()&nbsp;waits&nbsp;for&nbsp;all&nbsp;threads&nbsp;to&nbsp;be&nbsp;done&nbsp;using&nbsp;the<br/>&gt;barrier&nbsp;before&nbsp;destroying&nbsp;it.&nbsp;&nbsp;Without&nbsp;it,&nbsp;the&nbsp;storage&nbsp;for&nbsp;the&nbsp;barrier<br/>&gt;can&nbsp;be&nbsp;deallocated&nbsp;when&nbsp;timer_create()&nbsp;returns&nbsp;while&nbsp;the&nbsp;other&nbsp;thread&nbsp;is<br/>&gt;still&nbsp;using&nbsp;it&nbsp;inside&nbsp;the&nbsp;pthread_barrier_wait()&nbsp;implementation.<br/><br/>Sorry, I don't think add pthread_barrier_destroy in timer_create can solve this bug. In timer_create, b-&gt;_b_limit should be 1, and it will never meet the waiting condition (b-&gt;_b_limit &lt; 0) in pthread_barrier_destroy.<br/>And I think add additional synchronization to timer_create is a temporary method. Maybe some bugfix is needed in pthread_barrier_wait.<br/><br/>int pthread_barrier_destroy(pthread_barrier_t *b)<br/>{<br/> if (b-&gt;_b_limit &lt; 0) {<br/> if (b-&gt;_b_lock) {<br/> int v;<br/> a_or(&amp;b-&gt;_b_lock, INT_MIN);<br/> while ((v = b-&gt;_b_lock) &amp; INT_MAX)<br/> __wait(&amp;b-&gt;_b_lock, 0, v, 0);<br/> }<br/> __vm_wait();<br/> }<br/> return 0;<br/>}<br/><br/>Li<br/><br/><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<br/>Original&nbsp;Email<br/><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<br/><br/>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<br/>From:"Tavian&nbsp;Barnes"&lt;&nbsp;tavianator@...ianator.com&nbsp;&gt;;<br/>Sent&nbsp;Time:2024/7/11&nbsp;3:11<br/>To:"musl"&lt;&nbsp;musl@...ts.openwall.com&nbsp;&gt;;<br/>Cc&nbsp;recipient:"250200715"&lt;&nbsp;250200715@...com&nbsp;&gt;;<br/>Subject:[musl]&nbsp;RE:&nbsp;Maybe&nbsp;A&nbsp;Bug&nbsp;about&nbsp;timer_create&nbsp;and&nbsp;pthread_barrier_wait<br/><br/>(I&nbsp;cleaned&nbsp;up&nbsp;the&nbsp;HTML&nbsp;entities&nbsp;in&nbsp;this&nbsp;email,&nbsp;but&nbsp;please&nbsp;use&nbsp;plain&nbsp;text<br/>mode&nbsp;next&nbsp;time)<br/><br/>&gt;&nbsp;Hello:<br/>&gt;<br/>&gt;&nbsp;I&nbsp;had&nbsp;a&nbsp;low-probability&nbsp;crash&nbsp;in&nbsp;the&nbsp;child&nbsp;thread&nbsp;when&nbsp;using&nbsp;the<br/>&gt;&nbsp;timer_create&nbsp;interface.&nbsp;After&nbsp;debug,&nbsp;I&nbsp;found&nbsp;that&nbsp;the&nbsp;crash&nbsp;occured<br/>&gt;&nbsp;when&nbsp;the&nbsp;sub-thread&nbsp;accessed&nbsp;in&nbsp;code&nbsp;"if&nbsp;(b-&gt;_b_waiters)"&nbsp;which&nbsp;is&nbsp;a<br/>&gt;&nbsp;stack&nbsp;variable&nbsp;created&nbsp;in&nbsp;the&nbsp;main&nbsp;thread&nbsp;and&nbsp;passed&nbsp;to&nbsp;child&nbsp;thread<br/>&gt;&nbsp;by&nbsp;args.&nbsp;It&nbsp;looks&nbsp;like&nbsp;the&nbsp;main&nbsp;thread's&nbsp;timer_create&nbsp;has&nbsp;finished<br/>&gt;&nbsp;executing&nbsp;at&nbsp;this&nbsp;point,&nbsp;so&nbsp;the&nbsp;variables&nbsp;(start_args)&nbsp;on&nbsp;the&nbsp;stack<br/>&gt;&nbsp;have&nbsp;been&nbsp;cleaned&nbsp;up.&nbsp;I&nbsp;take&nbsp;a&nbsp;look&nbsp;at&nbsp;the&nbsp;pthread_barrier_wait&nbsp;code<br/>&gt;&nbsp;and&nbsp;I&nbsp;think&nbsp;it&nbsp;should&nbsp;be&nbsp;a&nbsp;scheduling&nbsp;problem&nbsp;in&nbsp;pthread_barrier_wait.<br/>&gt;<br/>&gt;&nbsp;Take&nbsp;the&nbsp;timer_create&nbsp;as&nbsp;an&nbsp;example,&nbsp;when&nbsp;the&nbsp;child&nbsp;thread&nbsp;is&nbsp;the<br/>&gt;&nbsp;first&nbsp;thread&nbsp;for&nbsp;"pthread_barrier_wait"&nbsp;and&nbsp;it&nbsp;is&nbsp;suspened&nbsp;after&nbsp;it<br/>&gt;&nbsp;executes&nbsp;the&nbsp;code&nbsp;"a_store(&amp;b-&gt;_b_lock,&nbsp;0)",&nbsp;then&nbsp;the&nbsp;main&nbsp;thread&nbsp;in<br/>&gt;&nbsp;timer_create&nbsp;will&nbsp;arrive&nbsp;as&nbsp;the&nbsp;last&nbsp;thread,&nbsp;it&nbsp;will&nbsp;nerver&nbsp;wait&nbsp;for<br/>&gt;&nbsp;the&nbsp;child&nbsp;thread&nbsp;to&nbsp;be&nbsp;rescheduled,&nbsp;the&nbsp;main&nbsp;thread&nbsp;can&nbsp;pass&nbsp;the<br/>&gt;&nbsp;barrier&nbsp;and&nbsp;continue&nbsp;execution,&nbsp;the&nbsp;args&nbsp;created&nbsp;in&nbsp;timer_create&nbsp;will<br/>&gt;&nbsp;be&nbsp;cleaned&nbsp;up.&nbsp;when&nbsp;the&nbsp;child&nbsp;thread&nbsp;is&nbsp;finally&nbsp;rescheduled,&nbsp;it&nbsp;access<br/>&gt;&nbsp;the&nbsp;"b-&gt;_b_waiters"&nbsp;which&nbsp;has&nbsp;already&nbsp;been&nbsp;cleaned&nbsp;up&nbsp;by&nbsp;main&nbsp;thread<br/>&gt;&nbsp;and&nbsp;the&nbsp;crash&nbsp;will&nbsp;occur.<br/>&gt;<br/>&gt;&nbsp;Is&nbsp;there&nbsp;a&nbsp;bug&nbsp;here?&nbsp;Looking&nbsp;forward&nbsp;to&nbsp;your&nbsp;reply.<br/>&gt;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;/*&nbsp;First&nbsp;thread&nbsp;to&nbsp;enter&nbsp;the&nbsp;barrier&nbsp;becomes&nbsp;the&nbsp;"instance&nbsp;owner"&nbsp;*/<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(!inst)&nbsp;{<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;struct&nbsp;instance&nbsp;new_inst&nbsp;=&nbsp;{&nbsp;0&nbsp;};<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;int&nbsp;spins&nbsp;=&nbsp;200;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;b-&gt;_b_inst&nbsp;=&nbsp;inst&nbsp;=&nbsp;&amp;new_inst;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;a_store(&amp;b-&gt;_b_lock,&nbsp;0);<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(b-&gt;_b_waiters)&nbsp;__wake(&amp;b-&gt;_b_lock,&nbsp;1,&nbsp;1);&nbsp;&nbsp;//&nbsp;crash&nbsp;here&nbsp;b-&gt;_b_waiters<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;while&nbsp;(spins--&nbsp;&amp;&amp;&nbsp;!inst-&gt;finished)<br/>&gt;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;/*&nbsp;First&nbsp;thread&nbsp;to&nbsp;enter&nbsp;the&nbsp;barrier&nbsp;becomes&nbsp;the&nbsp;"instance&nbsp;owner"&nbsp;*/<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(!inst)&nbsp;{<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;struct&nbsp;instance&nbsp;new_inst&nbsp;=&nbsp;{&nbsp;0&nbsp;};<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;int&nbsp;spins&nbsp;=&nbsp;200;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;b-&gt;_b_inst&nbsp;=&nbsp;inst&nbsp;=&nbsp;&amp;new_inst;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;a_store(&amp;b-&gt;_b_lock,&nbsp;0);<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;//&nbsp;when&nbsp;the&nbsp;child&nbsp;thread&nbsp;is&nbsp;the&nbsp;first&nbsp;thread&nbsp;and&nbsp;is&nbsp;scheduled&nbsp;out&nbsp;here<br/>&gt;<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;if&nbsp;(b-&gt;_b_waiters)&nbsp;__wake(&amp;b-&gt;_b_lock,&nbsp;1,&nbsp;1);<br/>&gt;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;while&nbsp;(spins--&nbsp;&amp;&amp;&nbsp;!inst-&gt;finished)<br/><br/>This&nbsp;looks&nbsp;like&nbsp;a&nbsp;real&nbsp;bug&nbsp;in&nbsp;timer_create()&nbsp;to&nbsp;me.&nbsp;&nbsp;Here's&nbsp;an&nbsp;untested<br/>fix:<br/><br/>From:&nbsp;Tavian&nbsp;Barnes&nbsp;<br/>Date:&nbsp;Wed,&nbsp;10&nbsp;Jul&nbsp;2024&nbsp;14:27:21&nbsp;-0400<br/>Subject:&nbsp;[PATCH]&nbsp;timer_create:&nbsp;destroy&nbsp;the&nbsp;barrier&nbsp;before&nbsp;returning<br/><br/>pthread_barrier_destroy()&nbsp;waits&nbsp;for&nbsp;all&nbsp;threads&nbsp;to&nbsp;be&nbsp;done&nbsp;using&nbsp;the<br/>barrier&nbsp;before&nbsp;destroying&nbsp;it.&nbsp;&nbsp;Without&nbsp;it,&nbsp;the&nbsp;storage&nbsp;for&nbsp;the&nbsp;barrier<br/>can&nbsp;be&nbsp;deallocated&nbsp;when&nbsp;timer_create()&nbsp;returns&nbsp;while&nbsp;the&nbsp;other&nbsp;thread&nbsp;is<br/>still&nbsp;using&nbsp;it&nbsp;inside&nbsp;the&nbsp;pthread_barrier_wait()&nbsp;implementation.<br/><br/>Link:&nbsp;https://www.openwall.com/lists/musl/2024/07/08/1<br/>---<br/>&nbsp;src/time/timer_create.c&nbsp;|&nbsp;1&nbsp;+<br/>&nbsp;1&nbsp;file&nbsp;changed,&nbsp;1&nbsp;insertion(+)<br/><br/>diff&nbsp;--git&nbsp;a/src/time/timer_create.c&nbsp;b/src/time/timer_create.c<br/>index&nbsp;9216b3ab..42c69848&nbsp;100644<br/>---&nbsp;a/src/time/timer_create.c<br/>+++&nbsp;b/src/time/timer_create.c<br/>@@&nbsp;-121,6&nbsp;+121,7&nbsp;@@&nbsp;int&nbsp;timer_create(clockid_t&nbsp;clk,&nbsp;struct&nbsp;sigevent&nbsp;*restrict&nbsp;evp,&nbsp;timer_t&nbsp;*restrict<br/>&nbsp;&nbsp;&nbsp;}<br/>&nbsp;&nbsp;&nbsp;td-&gt;timer_id&nbsp;=&nbsp;timerid;<br/>&nbsp;&nbsp;&nbsp;pthread_barrier_wait(&amp;args.b);<br/>+&nbsp;&nbsp;pthread_barrier_destroy(&amp;args.b);<br/>&nbsp;&nbsp;&nbsp;if&nbsp;(timerid&nbsp;&lt;&nbsp;0)&nbsp;return&nbsp;-1;<br/>&nbsp;&nbsp;&nbsp;*res&nbsp;=&nbsp;(void&nbsp;*)(INTPTR_MIN&nbsp;|&nbsp;(uintptr_t)td&gt;&gt;1);<br/>&nbsp;&nbsp;&nbsp;break;
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
