From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 9837 invoked from network); 22 May 2020 02:23:38 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 22 May 2020 02:23:38 -0000 Received: (qmail 10003 invoked by uid 550); 22 May 2020 02:23:36 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 32496 invoked from network); 22 May 2020 01:53:22 -0000 From: tangyizhou To: "dalias@aerifal.cx" CC: "musl@lists.openwall.com" , "Wanghui (John)" , "Huangshuai (OSLab)" Thread-Topic: [musl] Fix the return value of pthread_getschedparam in musl libc Thread-Index: AdYt7wSQbHOmwtVNSA+FzM3+dE/lswAjFtoAAD79gdA= Date: Fri, 22 May 2020 01:53:02 +0000 Message-ID: <4EB7132F5A45D144AC896FC29A83F192011628FB@DGGEML521-MBS.china.huawei.com> References: <4EB7132F5A45D144AC896FC29A83F192011602E1@DGGEML521-MBS.china.huawei.com> <20200520155046.GK1079@brightrain.aerifal.cx> In-Reply-To: <20200520155046.GK1079@brightrain.aerifal.cx> Accept-Language: en-US Content-Language: zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.166.213.6] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-CFilter-Loop: Reflected Subject: RE: [musl] Fix the return value of pthread_getschedparam in musl libc > I've tried to CC @huawei.com addresses before but your mail system is mis= configured to treat SPF neutral as fail. I don't know if this is something = you can get fixed, but getting it fixed would make it easier to=20 > participate in open source projects. Trying again now anyway in case it's= already fixed. I feel sorry for the strict and complicated information security policy of = our corporation. I received a mail from our mail system which says that sen= ding to dalias@aerifal.cx is timed out, and I don't know why.=20 > What is the situation under which SYS_sched_getparam can succeed but SYS_= sched_getscheduler can fail? The Linux man page documenting the > syscall: There is a timing issue between two syscalls. Think about a concurrent scen= ario: Thread A calls pthread_getschedparam() to query the information of thread B= . After SYS_sched_getparam succeeds and before SYS_sched_getscheduler start= s, thread B is killed, then SYS_sched_getparam returns -ESRCH. > If there is a good reason to consider the possibility of SYS_sched_getsch= eduler failing here, the patch has at least one bug and at least one minor = conformance issue. As written it examines *policy even if=20 > nonzero r from SYS_sched_getparam failing caused nothing to be written to= *policy (in which case it's accessing potentially-uninitialized memory, an= d clobbering the meaningful error code r with a meaningless=20 > one from uninitialized memory). The test would need to be inside the "if = (!r) {" block to be valid. The test should be inside the "if (!r) {" block. You're quite right, thanks= . > Also (minor conformance/policy issue) if the function is going to fail it= should not write to *policy at all, but to a temp var, and only copy to *p= olicy on success. The value of *policy is undefined when pthread_getschedparam() fails accor= ding to POSIX 2017 specification: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_getsched= param.html Only copying to *policy on success is a good programming practice. Here is = my new patch: diff --git a/src/thread/pthread_getschedparam.c b/src/thread/pthread_getsch= edparam.c index 1cba073d..4922cfa8 100644 --- a/src/thread/pthread_getschedparam.c +++ b/src/thread/pthread_getschedparam.c @@ -10,7 +10,11 @@ int pthread_getschedparam(pthread_t t, int *restrict pol= icy, struct sched_param } else { r =3D -__syscall(SYS_sched_getparam, t->tid, param); if (!r) { - *policy =3D __syscall(SYS_sched_getscheduler, t->ti= d); + r =3D -__syscall(SYS_sched_getscheduler, t->tid); + if (r <=3D 0) { + *policy =3D -r; + r =3D 0; + } } } UNLOCK(t->killlock); Yizhou -----Original Message----- From: dalias@aerifal.cx [mailto:dalias@aerifal.cx]=20 Sent: Wednesday, May 20, 2020 11:51 PM To: tangyizhou Cc: musl@lists.openwall.com; Wanghui (John) ; Huan= gshuai (OSLab) Subject: Re: [musl] Fix the return value of pthread_getschedparam in musl l= ibc On Wed, May 20, 2020 at 02:09:25AM +0000, tangyizhou wrote: > Hi, >=20 > >From the latest upstream, I find the return value of > >pthread_getschedparam() is wrong when syscall sched_getparam succeeds=20 > >but sched_getscheduler fails. It is rare to see this situation. When=20 > >it happens, pthread_getschedparam() will return r which must be 0,=20 > >that means a success according to POSIX 2017 specification. It's=20 > >wrong. >=20 > Also, POSIX 2017 specification says an error number shall be returned=20 > to indicate the error. I think returning -*policy is appropriate=20 > because syscall sched_getscheduler returns an negative error code in=20 > kernel like Linux when it fails. What is the situation under which SYS_sched_getparam can succeed but SYS_sc= hed_getscheduler can fail? The Linux man page documenting the syscall: http://man7.org/linux/man-pages/man2/sched_setscheduler.2.html does not specify any errors meaningful for SYS_sched_getscheduler other tha= n ESRCH which is already precluded by the previous syscall having succeeded= . > Here is my patch. I want to be Cc'd on replies. Thanks. I've tried to CC @huawei.com addresses before but your mail system is misco= nfigured to treat SPF neutral as fail. I don't know if this is something yo= u can get fixed, but getting it fixed would make it easier to participate i= n open source projects. Trying again now anyway in case it's already fixed. > diff --git a/src/thread/pthread_getschedparam.c=20 > b/src/thread/pthread_getschedparam.c > index 1cba073d..8203d609 100644 > --- a/src/thread/pthread_getschedparam.c > +++ b/src/thread/pthread_getschedparam.c > @@ -12,6 +12,9 @@ int pthread_getschedparam(pthread_t t, int *restrict po= licy, struct sched_param > if (!r) { > *policy =3D __syscall(SYS_sched_getscheduler, t->= tid); > } > + if (*policy < 0) { > + r =3D -*policy; > + } > } > UNLOCK(t->killlock); > return r; If there is a good reason to consider the possibility of SYS_sched_getsched= uler failing here, the patch has at least one bug and at least one minor co= nformance issue. As written it examines *policy even if nonzero r from SYS_= sched_getparam failing caused nothing to be written to *policy (in which ca= se it's accessing potentially-uninitialized memory, and clobbering the mean= ingful error code r with a meaningless one from uninitialized memory). The = test would need to be inside the "if (!r) {" block to be valid. Also (minor conformance/policy issue) if the function is going to fail it s= hould not write to *policy at all, but to a temp var, and only copy to *pol= icy on success. Rich