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

[SR-11354] Foundation.Process inherits file descriptors into the child process #3291

Closed
weissi opened this issue Aug 22, 2019 · 9 comments
Closed

Comments

@weissi
Copy link
Member

weissi commented Aug 22, 2019

Previous ID SR-11354
Radar rdar://problem/54600455
Original Reporter @weissi
Type Bug
Status Resolved
Resolution Done

Attachment: Download

Additional Detail from JIRA
Votes 1
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: 6bb31ba0bafc6fc1de60d481ac760a2d

Issue Description:

The following program when compiled and run like swiftc test.swift && ./test should print

ok1
the line above should read 'ok1'

which is does on Darwin. The reason is that Foundation.Process is supposed to close the parents file descriptors so that they're not inherited into the child.

However, on Linux, this is not the case:

$ docker run -it --rm -v "$PWD:$PWD" -w "$PWD" swift:5.0.2 bash -c 'swiftc test.swift && ./test'
ok1
THIS IS BAD
the line above should read 'ok1'

#if os(Linux)
import Glibc
#else
import Darwin
#endif
import Foundation


if let stdout2 = CommandLine.arguments.dropFirst().first.flatMap(CInt.init) {
    write(stdout2, "THIS IS BAD\n", 12)
    print("the line above should read 'ok1'")
} else {
    let stdout2 = dup(1)
    write(stdout2, "ok1\n", 4)
    let p = Process()
    p.executableURL = URL(fileURLWithPath: CommandLine.arguments.first!)
    p.arguments = [ "\(stdout2)" ]
    try! p.run()
    p.waitUntilExit()
}
@weissi
Copy link
Member Author

weissi commented Aug 22, 2019

@swift-ci create

@millenomi
Copy link
Contributor

That's because Process on Darwin uses the non-portable POSIX_SPAWN_CLOEXEC_DEFAULT flag to have the kernel do it, which we didn't port over.

Apparently, on Linux, we can implement it via the approach in point 2 of https://docs.fedoraproject.org/en-US/Fedora_Security_Team/1/html/Defensive_Coding/sect-Defensive_Coding-Tasks-Descriptors-Child_Processes.html.

@millenomi
Copy link
Contributor

@compnerd gwenm (JIRA User) the above approach has a POSIX portable approximation (add file close ops for all fds from 0 to FD_SETSIZE - 1), but Windows has no fds; is Windows affected similarly? (IIRC no?)

@weissi
Copy link
Member Author

weissi commented Aug 22, 2019

@millenomi the only thing I think will work is to set POSIX_SPAWN_CLOEXEC_DEFAULT on Darwin and do

        for fd in 3..<getdtablesize() {
            posix(_CFPosixSpawnFileActionsAddClose(fileActions, fd))
        }

on Linux. Closing up to getdtablesize() is the standard thing to do between fork and execve.

@millenomi
Copy link
Contributor

The links are outdated, then. Sigh, Internet.

@weissi
Copy link
Member Author

weissi commented Aug 22, 2019

@millenomi something (needs a lot of cleanup) like this: #2485

@swift-ci
Copy link
Contributor

swift-ci commented Sep 5, 2019

Comment by Gwen Mittertreiner (JIRA)

I checked this on Windows (with some modification: write on Windows asserts by default if given a bad fd), it works as excepted.

import Foundation
import MSVCRT
import WinSDK
func dontFailPlease(e: UnsafePointer<UInt16>?, f: UnsafePointer<UInt16>?, file: UnsafePointer<UInt16>?, line: UInt32, pReserved: UInt) { }

if let stdout2 = CommandLine.arguments.dropFirst().first.flatMap(CInt.init) {
  _set_invalid_parameter_handler(dontFailPlease)
  write(stdout2, "THIS IS BAD\n", 12)
  print("the line above should read 'ok1'")
} else {
  let stdout2 = dup(1)
  write(stdout2, "ok1\n", 4)
  let p = Process()
  p.executableURL = URL(fileURLWithPath: CommandLine.arguments.first!)
  p.arguments = [ "\(stdout2)" ]
  try! p.run()
  p.waitUntilExit()
}

Running it:

C:\Users\gwenm\tmp\fds ▶ ./main.exe
ok1
the line above should read 'ok1'

@swift-ci
Copy link
Contributor

swift-ci commented Dec 1, 2020

Comment by Gregor Milos (Grzegorz Miłoś) (JIRA)

@weissi this should be closed now that the PR is merged? #2485

@weissi
Copy link
Member Author

weissi commented Dec 1, 2020

Yes, thanks Gregor!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants