[musl] Re: libc-test issue with sem_close

[musl] Re: libc-test issue with sem_close

[Szabolcs Nagy]

* Matus Kysel <mkysel@tachyum.com> [2021-04-26 09:49:35 +0000]:
> Hi Szabolcs,
> 
> I am using your library as part of my validation suite and recently I have updated it to latest master, but the sem_close test is not buildable for me as it is missing fcntl.h header. I have attached possible fix, because I did not found how to contribute to you repo.

ccing musl, libc-test is mostly discussed there.

posix says

"Inclusion of the <semaphore.h> header may make visible symbols defined in the <fcntl.h> and <time.h> headers."

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/semaphore.h.html

which sounds optional, not guaranteed visibility of O_ open flags in
semaphore.h, but sem_open is expected to be usable with only semaphore.h

https://pubs.opengroup.org/onlinepubs/9699919799/functions/sem_open.html

i'm not sure what is the strict standard requirement here, without
clarification i consider it a libc header issue if an api is not usable
with just the header where it is declared.

Re: [musl] Re: libc-test issue with sem_close

[Érico Nogueira]

Em 27/04/2021 16:55, Szabolcs Nagy escreveu:
> * Matus Kysel <mkysel@tachyum.com> [2021-04-26 09:49:35 +0000]:
>> Hi Szabolcs,
>>
>> I am using your library as part of my validation suite and recently I have updated it to latest master, but the sem_close test is not buildable for me as it is missing fcntl.h header. I have attached possible fix, because I did not found how to contribute to you repo.
> 
> ccing musl, libc-test is mostly discussed there.
> 
> posix says
> 
> "Inclusion of the <semaphore.h> header may make visible symbols defined in the <fcntl.h> and <time.h> headers."
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/semaphore.h.html
> 
> which sounds optional, not guaranteed visibility of O_ open flags in
> semaphore.h, but sem_open is expected to be usable with only semaphore.h
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sem_open.html
> 
> i'm not sure what is the strict standard requirement here, without
> clarification i consider it a libc header issue if an api is not usable
> with just the header where it is declared.
> 

The linux man page for sem_open(3) says including <fcntl.h> is necessary 
for the flags, and indeed the test fails to build on glibc.

   The oflag argument specifies flags that control the operation of the
   call.  (Definitions of the flags values can be obtained by including
   <fcntl.h>.)

sem_open(name, 0) works without <fcntl.h>, otherwise it's necessary.

Taking advantage of the fact that the regressions/sem_close-unmap test 
has been brought up, it has some interesting results:

On glibc 2.32:
- works with dynamic linking
- segfaults in sem_open() with static linking (might be something wrong 
with the build?)

On musl 1.2.2:
- segfaults most of the time (always?)

On musl master (aad50fcd791e009961621ddfbe3d4c245fd689a3):
- completes successfully most of the time, segfaults ever so often 
(which would lead me to assume the fix from 
f70375df85d26235a45e74559afd69be59e5ff99 wasn't enough)

Note: for musl master I ran it as

   /path/muslrepo/lib/libc.so /path/libc-tests/.../sem_close-unmap.exe

Cheers,
Érico

Re: [musl] Re: libc-test issue with sem_close

[Érico Nogueira]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

Em 27/04/2021 17:33, Érico Nogueira escreveu:
> Em 27/04/2021 16:55, Szabolcs Nagy escreveu:
>> * Matus Kysel <mkysel@tachyum.com> [2021-04-26 09:49:35 +0000]:
>>> Hi Szabolcs,
>>>
>>> I am using your library as part of my validation suite and recently I 
>>> have updated it to latest master, but the sem_close test is not 
>>> buildable for me as it is missing fcntl.h header. I have attached 
>>> possible fix, because I did not found how to contribute to you repo.
>>
>> ccing musl, libc-test is mostly discussed there.
>>
>> posix says
>>
>> "Inclusion of the <semaphore.h> header may make visible symbols 
>> defined in the <fcntl.h> and <time.h> headers."
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/semaphore.h.html 
>>
>>
>> which sounds optional, not guaranteed visibility of O_ open flags in
>> semaphore.h, but sem_open is expected to be usable with only semaphore.h
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/sem_open.html
>>
>> i'm not sure what is the strict standard requirement here, without
>> clarification i consider it a libc header issue if an api is not usable
>> with just the header where it is declared.
>>
> 
> The linux man page for sem_open(3) says including <fcntl.h> is necessary 
> for the flags, and indeed the test fails to build on glibc.
> 
>   The oflag argument specifies flags that control the operation of the
>   call.  (Definitions of the flags values can be obtained by including
>   <fcntl.h>.)
> 
> sem_open(name, 0) works without <fcntl.h>, otherwise it's necessary.
> 
> Taking advantage of the fact that the regressions/sem_close-unmap test 
> has been brought up, it has some interesting results:
> 
> On glibc 2.32:
> - works with dynamic linking
> - segfaults in sem_open() with static linking (might be something wrong 
> with the build?)
> 
> On musl 1.2.2:
> - segfaults most of the time (always?)

My mistake, I was on 1.2.1.

> 
> On musl master (aad50fcd791e009961621ddfbe3d4c245fd689a3):
> - completes successfully most of the time, segfaults ever so often 
> (which would lead me to assume the fix from 
> f70375df85d26235a45e74559afd69be59e5ff99 wasn't enough)

The sem_create() calls can sometimes fail here with EINVAL because 
sem_open is being called with the wrong number of arguments. Included 
patch to fix it.

> 
> Note: for musl master I ran it as
> 
>   /path/muslrepo/lib/libc.so /path/libc-tests/.../sem_close-unmap.exe
> 
> Cheers,
> Érico


[-- Attachment #2: 0001-fix-sem_close-regression-test.patch --]
[-- Type: text/x-patch, Size: 1163 bytes --]

From bb113fe0b0e209681b1c0d6317440bf64f57ea80 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=89rico=20Nogueira?= <ericonr@disroot.org>
Date: Tue, 27 Apr 2021 17:46:17 -0300
Subject: [PATCH] fix sem_close regression test

sem_close with O_CREAT takes 4 parameters, not 3. The last parameter,
the initial value for the semaphore, had an arbitrary value from
whatever was on the stack, and lead to spurious failures with EINVAL
when it happened to be greater than SEM_VALUE_MAX. Since there isn't
error checking in this test, this lead to a segfault in the first
sem_post call.
---
 src/regression/sem_close-unmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/regression/sem_close-unmap.c b/src/regression/sem_close-unmap.c
index f1e51d9..73c53e3 100644
--- a/src/regression/sem_close-unmap.c
+++ b/src/regression/sem_close-unmap.c
@@ -8,7 +8,7 @@ int main()
 	char buf[] = "mysemXXXXXX";
 	if (!mktemp(buf)) return 1;
 	// open twice
-	sem_t *sem = sem_open(buf, O_CREAT|O_EXCL, 0600);
+	sem_t *sem = sem_open(buf, O_CREAT|O_EXCL, 0600, 0);
 	sem_open(buf, 0);
 	sem_unlink(buf);
 	// close once
-- 
2.31.1

Re: [musl] Re: libc-test issue with sem_close

[Szabolcs Nagy]

* Érico Nogueira <ericonr@disroot.org> [2021-04-27 17:49:45 -0300]:
> Em 27/04/2021 17:33, Érico Nogueira escreveu:
> > Em 27/04/2021 16:55, Szabolcs Nagy escreveu:
> > > * Matus Kysel <mkysel@tachyum.com> [2021-04-26 09:49:35 +0000]:
> > > > Hi Szabolcs,
> > > > 
> > > > I am using your library as part of my validation suite and
> > > > recently I have updated it to latest master, but the sem_close
> > > > test is not buildable for me as it is missing fcntl.h header. I
> > > > have attached possible fix, because I did not found how to
> > > > contribute to you repo.
> > > 
> > > ccing musl, libc-test is mostly discussed there.
> > > 
> > > posix says
> > > 
> > > "Inclusion of the <semaphore.h> header may make visible symbols
> > > defined in the <fcntl.h> and <time.h> headers."
> > > 
> > > https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/semaphore.h.html
> > > 
> > > 
> > > which sounds optional, not guaranteed visibility of O_ open flags in
> > > semaphore.h, but sem_open is expected to be usable with only semaphore.h
> > > 
> > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/sem_open.html
> > > 
> > > i'm not sure what is the strict standard requirement here, without
> > > clarification i consider it a libc header issue if an api is not usable
> > > with just the header where it is declared.
> > > 
> > 
> > The linux man page for sem_open(3) says including <fcntl.h> is necessary
> > for the flags, and indeed the test fails to build on glibc.
> > 
> >   The oflag argument specifies flags that control the operation of the
> >   call.  (Definitions of the flags values can be obtained by including
> >   <fcntl.h>.)
> > 
> > sem_open(name, 0) works without <fcntl.h>, otherwise it's necessary.
> > 
> > Taking advantage of the fact that the regressions/sem_close-unmap test
> > has been brought up, it has some interesting results:
> > 
> > On glibc 2.32:
> > - works with dynamic linking
> > - segfaults in sem_open() with static linking (might be something wrong
> > with the build?)
> > 
> > On musl 1.2.2:
> > - segfaults most of the time (always?)
> 
> My mistake, I was on 1.2.1.
> 
> > 
> > On musl master (aad50fcd791e009961621ddfbe3d4c245fd689a3):
> > - completes successfully most of the time, segfaults ever so often
> > (which would lead me to assume the fix from
> > f70375df85d26235a45e74559afd69be59e5ff99 wasn't enough)
> 
> The sem_create() calls can sometimes fail here with EINVAL because sem_open
> is being called with the wrong number of arguments. Included patch to fix
> it.

thanks for looking into this.

based on this i applied both patches (include fcntl.h and the argument fix).

> From bb113fe0b0e209681b1c0d6317440bf64f57ea80 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=C3=89rico=20Nogueira?= <ericonr@disroot.org>
> Date: Tue, 27 Apr 2021 17:46:17 -0300
> Subject: [PATCH] fix sem_close regression test
> 
> sem_close with O_CREAT takes 4 parameters, not 3. The last parameter,
> the initial value for the semaphore, had an arbitrary value from
> whatever was on the stack, and lead to spurious failures with EINVAL
> when it happened to be greater than SEM_VALUE_MAX. Since there isn't
> error checking in this test, this lead to a segfault in the first
> sem_post call.
> ---
>  src/regression/sem_close-unmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/regression/sem_close-unmap.c b/src/regression/sem_close-unmap.c
> index f1e51d9..73c53e3 100644
> --- a/src/regression/sem_close-unmap.c
> +++ b/src/regression/sem_close-unmap.c
> @@ -8,7 +8,7 @@ int main()
>  	char buf[] = "mysemXXXXXX";
>  	if (!mktemp(buf)) return 1;
>  	// open twice
> -	sem_t *sem = sem_open(buf, O_CREAT|O_EXCL, 0600);
> +	sem_t *sem = sem_open(buf, O_CREAT|O_EXCL, 0600, 0);
>  	sem_open(buf, 0);
>  	sem_unlink(buf);
>  	// close once
> -- 
> 2.31.1
>