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.

Sign up for GitHub

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

Jump to bottom

core::run and rust_run_program.cpp do unsafe and leaky things on windows #5976

Closed
Dretch opened this issue Apr 20, 2013 · 2 comments
Closed

core::run and rust_run_program.cpp do unsafe and leaky things on windows #5976

Dretch opened this issue Apr 20, 2013 · 2 comments
Labels
O-windows Operating system: Windows

Comments

@Dretch
Copy link
Contributor

Dretch commented Apr 20, 2013

When starting a child process on Windows then rust_run_program.cpp casts a process HANDLE to int and then core::run pretends that it is really a process id (a pid_t).

This is bad because (on Windows):

  • The HANDLE never gets closed (it leaks).
  • It means calling core::os::waitpid or core::run::waitpid with a pid_t that wasn't retrieved from rust_run_program will result in undefined behaviour.
  • it means that calling core::run::Program.get_id returns a value that isn't really the process id.

I will send a pull request to address these issues, unless there are objections.
@graydon
Copy link
Contributor

graydon commented Apr 20, 2013

Please do! Sorry about the current behavior

@Dretch Dretch mentioned this issue Apr 22, 2013
Closed
Dretch pushed a commit to Dretch/rust that referenced this issue Apr 23, 2013
Fix issue rust-lang#5976 - HANDLE leaks and undefined/bad behavour
91aeecf 
on windows.
bors added a commit that referenced this issue Apr 23, 2013
@bors
auto merge of #6010 : Dretch/rust/run-windows, r=brson
8708e0c 
This fixes #5976.

It also removes `os::waitpid` in favour of (the existing) `run::waitpid`. I included this change because I figured it is kind of related.

r?
@Dretch Dretch closed this as completed Apr 24, 2013
@mzabaluev mzabaluev mentioned this issue Apr 28, 2013
Closed
@mzabaluev
Copy link
Contributor

The fix bisects as the cause of #6096.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
O-windows Operating system: Windows
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@graydon @mzabaluev @Dretch