Re: Fwd: gnulib posix_spawn_file_actions_addclose

[Bruno Haible]

I committed:
> 2022-09-07  Bruno Haible  
> 
>   posix_spawn_file_actions_addclose tests: Avoid test failure on musl.
>   Reported by Valery Ushakov  in
>   .
>   * modules/posix_spawn_file_actions_addclose-tests (configure.ac): Invoke
>   gl_MUSL_LIBC.
>   * tests/test-posix_spawn_file_actions_addclose.c (main): Skip one of the
>   tests on musl libc.

This patch was incomplete. Here's the needed addendum.


2022-09-09  Bruno Haible  

posix_spawn_file_actions_addclose tests: Fix mistake from 2022-09-07.
* modules/posix_spawn_file_actions_addclose-tests (Files): Add
m4/musl.m4.

diff --git a/modules/posix_spawn_file_actions_addclose-tests 
b/modules/posix_spawn_file_actions_addclose-tests
index 701e04dc17..4cc8623c16 100644
--- a/modules/posix_spawn_file_actions_addclose-tests
+++ b/modules/posix_spawn_file_actions_addclose-tests
@@ -2,6 +2,7 @@ Files:
 tests/test-posix_spawn_file_actions_addclose.c
 tests/signature.h
 tests/macros.h
+m4/musl.m4
 
 Depends-on:
 getdtablesize

Re: Fwd: gnulib posix_spawn_file_actions_addclose

[Bruno Haible]

Valery Ushakov wrote:
> However for the addclose case the [1] says in the ERRORS section:
> 
>   The posix_spawn_file_actions_addopen() function shall fail if:
> 
>   [EBADF]
>   The value specified by fildes is negative or greater than or
>   equal to {OPEN_MAX}.
> 
>   The posix_spawn_file_actions_addclose() function shall fail if:
> 
>   [EBADF]
>   The value specified by fildes is negative. 

Ah, I see: I initially wrote the test based on the old specification [0].
Then POSIX changed the part about posix_spawn_file_actions_addclose [1].
Then, in 2021 I acknowledged this through commit [2].

At the time, musl libc still had the bug regarding the negative file
descriptors; therefore the configure test said "no", gnulib added its
replacement, and the unit test succeeded.

Now, you have discovered that musl libc no longer has the bug regarding
the negative file descriptors; therefore the configure test says "yes",
gnulib no longer overrides the function, and the unit test now fails.

The patch below should fix it.

[0] 
https://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn_file_actions_addclose.html
[1] 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_addclose.html
[2] 
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=36a838b081de0d2c62970df5b4b8d736d3aebe1d


2022-09-07  Bruno Haible  

posix_spawn_file_actions_addclose tests: Avoid test failure on musl.
Reported by Valery Ushakov  in
.
* modules/posix_spawn_file_actions_addclose-tests (configure.ac): Invoke
gl_MUSL_LIBC.
* tests/test-posix_spawn_file_actions_addclose.c (main): Skip one of the
tests on musl libc.

diff --git a/modules/posix_spawn_file_actions_addclose-tests 
b/modules/posix_spawn_file_actions_addclose-tests
index b115e3dfca..701e04dc17 100644
--- a/modules/posix_spawn_file_actions_addclose-tests
+++ b/modules/posix_spawn_file_actions_addclose-tests
@@ -9,6 +9,7 @@ posix_spawn_file_actions_init
 posix_spawn_file_actions_destroy
 
 configure.ac:
+gl_MUSL_LIBC
 
 Makefile.am:
 TESTS += test-posix_spawn_file_actions_addclose
diff --git a/tests/test-posix_spawn_file_actions_addclose.c 
b/tests/test-posix_spawn_file_actions_addclose.c
index c5238c3e38..d0ab33d14f 100644
--- a/tests/test-posix_spawn_file_actions_addclose.c
+++ b/tests/test-posix_spawn_file_actions_addclose.c
@@ -55,12 +55,14 @@ main (void)
 ASSERT (posix_spawn_file_actions_addclose (, -1) == EBADF);
   }
   /* This behaviour is not mandated by POSIX, but happens to pass on all
- platforms.  */
+ platforms except musl libc.  */
+#if !defined MUSL_LIBC
   {
 int bad_fd = big_fd ();
 errno = 0;
 ASSERT (posix_spawn_file_actions_addclose (, bad_fd) == EBADF);
   }
+#endif
 
   posix_spawn_file_actions_destroy ();
 

Re: Fwd: gnulib posix_spawn_file_actions_addclose

[Valery Ushakov]

On Wed, Sep 07, 2022 at 19:29:11 +0200, Bruno Haible wrote:

> > The addclose configure test succeeds b/c of this commit:
> > 
> >   
> > https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=36a838b081de0d2c62970df5b4b8d736d3aebe1d
> > 
> > that makes configure test a negative fd instead of a large fd.
> 
> Yes.
> 
> > However tests/test-posix_spawn_file_actions_addclose.c still tests for
> > a large fd too and fails that test.
> 
> Yes. I believe the test verifies one of the assertions by POSIX [1].
> 
> > I wonder if either test-posix_spawn_file_actions_addclose.c should not
> > check for the "big" fd (and neither should spawn_faction_addclose.c
> > too?)  or the configure check should be reverted to check for the
> > "big" fd to make the two match.
> 
> I believe it's a POSIX compliance bug in musl libc, but Rich Felker
> disagrees. [2][3][4]
> 
> [1] 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_addclose.html
> [2] https://www.openwall.com/lists/musl/2019/03/24/2
> [3] https://www.openwall.com/lists/musl/2019/03/24/5
> [4] https://www.openwall.com/lists/musl/2019/03/24/7

I can see that line of reasoning for the addopen case, though I would
need to read and think a bit more to make an informed comment.

However for the addclose case the [1] says in the ERRORS section:

  The posix_spawn_file_actions_addopen() function shall fail if:

  [EBADF]
  The value specified by fildes is negative or greater than or
  equal to {OPEN_MAX}.

  The posix_spawn_file_actions_addclose() function shall fail if:

  [EBADF]
  The value specified by fildes is negative. 


So I read it that the OPEN_MAX limit is - by explicit omission if you
excuse the oxymoron - not required for addclose (and that commit even
added the comment to that effect to the actual test).

That also seems to make sense.  In a real program the fd you pass to
addclose is some actual fd you obtained previously, so it must be
valid by construction.  If the fd is invalid *already*, in whichever
way, the child process will not be affected, b/c in any case it will
not have that fd open when it runs.  So even EBADF for negative
numbers is kinda redundant.


Anyway, my actual point is that the configure test and the actual test
in the tests subdir are not consistent.  I don't have any strong
preference for which way that inconsistency is resolved (though I do
lean towards the "negative only" side), but I think it would be nice
to resolve it one way or the other.

Thanks.

-uwe

Re: Fwd: gnulib posix_spawn_file_actions_addclose

[Bruno Haible]

Hi Uwe,

> It looks like you are the author of the posix_spawn
> implementation in gnulib

Ulrich Drepper wrote most of the part for the Unix platforms;
I wrote the implementation for native Windows.

> so I wanted to ask a question privately before spamming bug-gnulib.

Better direct all questions to the mailing list.
1. So that you don't have to wait in times when I am not available.
2. So that the discussions are available for later reference.

> On a systems with musl-1.2.3 for its libc the gnulib configure (as
> part of m4 1.4.19) detects:
> 
> $ grep spawn .log.configure 
> checking for spawn.h... yes
> checking for posix_spawn_file_actions_addchdir_np... yes
> checking for posix_spawn_file_actions_addchdir... no
> checking for library containing posix_spawn... none required
> checking for posix_spawn... yes
> checking whether posix_spawn is declared... yes
> checking whether posix_spawn works... yes
> checking whether posix_spawn rejects scripts without shebang... yes
> checking whether posix_spawnp rejects scripts without shebang... yes
> checking whether posix_spawnattr_setschedpolicy is supported... yes
> checking whether posix_spawnattr_setschedparam is supported... yes
> checking for posix_spawnattr_t... yes
> checking for posix_spawn_file_actions_t... yes
> checking whether posix_spawn_file_actions_addclose works... yes
> checking whether posix_spawn_file_actions_adddup2 works... no
> checking whether posix_spawn_file_actions_addopen works... no
> 
> 
> The addclose configure test succeeds b/c of this commit:
> 
>   
> https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=36a838b081de0d2c62970df5b4b8d736d3aebe1d
> 
> that makes configure test a negative fd instead of a large fd.

Yes.

> However tests/test-posix_spawn_file_actions_addclose.c still tests for
> a large fd too and fails that test.

Yes. I believe the test verifies one of the assertions by POSIX [1].

> I wonder if either test-posix_spawn_file_actions_addclose.c should not
> check for the "big" fd (and neither should spawn_faction_addclose.c
> too?)  or the configure check should be reverted to check for the
> "big" fd to make the two match.

I believe it's a POSIX compliance bug in musl libc, but Rich Felker
disagrees. [2][3][4]

Bruno

[1] 
https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_addclose.html
[2] https://www.openwall.com/lists/musl/2019/03/24/2
[3] https://www.openwall.com/lists/musl/2019/03/24/5
[4] https://www.openwall.com/lists/musl/2019/03/24/7