Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cbits/fork-exec: Don't dup2 identical fds #250

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented Jun 14, 2022

Darwin violates POSIX by making
dup2(x,x), which should be a no-op, error. Consequently, we must take
care not to dup2 in such cases.

We had already made this change in the posix_spawnp codepath but I had
assumed that this only affected posix_spawnp, not the dup2 system
call itself.

See also: #214

Darwin violates POSIX by making
`dup2(x,x)`, which should be a no-op, error. Consequently, we must take
care not to `dup2` in such cases.

We had already made this change in the `posix_spawnp` codepath but I had
assumed that this *only* affected `posix_spawnp`, not the `dup2` system
call itself.
@psibi
Copy link
Contributor

psibi commented Jun 14, 2022

I compiled Stack using this patch, but I'm still able to reproduce the issue when running stack's integration test. Some relevant logs:

/Users/ec2-user/.stack/programs/x86_64-osx/ghc-9.2.3/bin/ghc-9.2.3: startProcess: dup2: invalid argument (Bad file descriptor)
Main.hs: Exited with exit code: ExitFailure 1
CallStack (from HasCallStack):
  error, called at /Users/ec2-user/stack/test/integration/lib/StackTest.hs:63:34 in main:StackTest
  stack, called at /Users/ec2-user/stack/test/integration/tests/111-custom-snapshot/Main.hs:4:8 in main:Main

Unfortunately I haven't been able to find a minimal reproducible example.

@bgamari
Copy link
Contributor Author

bgamari commented Jun 14, 2022

Hmm, this is quite mysterious. Are you certain that you are linking against the correct process? Would it be possible to collect dtruss output from this execution?

@psibi
Copy link
Contributor

psibi commented Jun 15, 2022

I mis-applied your patch, I rebased your changes against master branch and it indeed fixes the issue. Relevant logs:

End of log for 111-custom-snapshot
Successful tests:
- 111-custom-snapshot

No failures!

Although I'm still able to reproduce the posix_spawnp issue which I have described in the issue.

Copy link
Contributor

@psibi psibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the dup2 issue that was being reproduced in the stack codebase.

snoyberg added a commit that referenced this pull request Jun 15, 2022
@snoyberg snoyberg merged commit 5546928 into haskell:master Jun 15, 2022
@snoyberg
Copy link
Collaborator

Thanks @bgamari!

mpilgrem added a commit to commercialhaskell/stack that referenced this pull request Aug 3, 2022
mpilgrem added a commit to commercialhaskell/stack that referenced this pull request Aug 12, 2022
mpilgrem added a commit to commercialhaskell/stack that referenced this pull request Aug 12, 2022
mpilgrem pushed a commit to commercialhaskell/stack that referenced this pull request Aug 13, 2022
Bump to nightly-2022-07-30

Remove redundant import in Stack.Package

Bump to nightly-2022-08-02 (GHC 9.2.4)

Fix 4101-dependency-tree test

Maybe fix the ghc-install-hooks test

process-1.6.15.0 now available

Includes fix haskell/process#250
Mistuke pushed a commit to Mistuke/process that referenced this pull request Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants