Skip to content

Fix leaks/undefined behavour in core::run on windows, and remove os::waitpid - #6010

Closed
Dretch wants to merge 4 commits into
rust-lang:incomingfrom
Dretch:run-windows
Closed

Dretch wants to merge 4 commits into
rust-lang:incomingfrom
Dretch:run-windows

Conversation

@Dretch

@Dretch Dretch commented Apr 22, 2013

Copy link
Copy Markdown
Contributor

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?

@brson

brson commented Apr 23, 2013

Copy link
Copy Markdown
Contributor

Nice fixes. Thanks.

@Dretch

Dretch commented Apr 23, 2013

Copy link
Copy Markdown
Contributor Author

@graydon

Might consider moving the sole waitpid (I agree duplication is bad!) back to os, as it's not running-program-specific

I think the waitpid function might actually make more sense if it was a method on core::run::Program.

On unix a process can only wait on its children, and allowing calls to waitpid without going through Program could lead to race conditions because after calling waitpid the pid can be re-used which means that the Program finish method (or the Program destructor...) might actually wait on a different process.

On windows a process can wait (i.e. WaitForSingleObject) on the children of other processes, but I guess it would be OK to not present an API for doing this via core::run, because you can't do it on unix.

It is probably also worth mentioning that the ProgRepr destructor does a waitpid to wait for the process to exit, unless the process has already been waited on. I think this might be a bit unexpected, so maybe the documentation for core::run should make it more obvious. It might also be possible to avoid waiting for the process to exit in the destructor, but it looks like that could involve (on unix) signal handlers and/or global mutable state to avoid race conditions and/or zombie processes.

Might consider changing the structure return to an out-pointer, as our FFI sometimes has trouble with structure returns. Lame excuse but possibly a risk at this point.
Might consider rewriting run_program.cpp in rust, period. I think the unsafe dialect is strong enough to handle it now. It wasn't when run_program was written.

I agree that sounds sensible and I shall consider attempting follow-ups.

gareth added 4 commits April 23, 2013 21:23
 - The return value meant different things on different
   platforms (on windows, it was the exit code, on unix it was
   the status information returned from waitpid).
 - It was undocumented.
 - There also exists run::waitpid, which does much the same
   thing but has a more consistent return value and also some
   documentation.
@Dretch

Dretch commented Apr 23, 2013

Copy link
Copy Markdown
Contributor Author

I rebased because BORS told me to ;)

r?

bors added a commit that referenced this pull request Apr 23, 2013
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?
@bors bors closed this Apr 23, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Nov 20, 2020
Add a case to `lint_search_is_some` to handle searching strings

Fixes: rust-lang#6010

This adds a lint which recommends using `contains()` instead of `find()` followed by `is_some()` on strings as suggested in rust-lang#6010.

This was added as an additional case to
https://github.com/rust-lang/rust-clippy/blob/5af88e3c2d8cc4fb74a0e455381669930ee3a31a/clippy_lints/src/methods/mod.rs#L3037

I would really appreciate any comments/suggestions for my code!

changelog: Added case to `lint_search_is_some` to handle searching strings
U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
6010: Avoid checking all ancestors and fix mis-completion r=jonas-schievink a=oxalica

Refactor the logic of `completion_match` to check deterministic number of ancestors instead of `token.ancestors().find_map()`.
This should fix wrong completions (rust-lang/rust-analyzer#5976 (comment)) and hopefully make completion to be faster (rust-lang#6004).

More play and test? @jonas-schievink @hammypants 

If this patch works, we can avoid the revert rust-lang#6005 . 😞

Co-authored-by: oxalica <oxalicc@pm.me>
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