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

os/exec: export thread id come from CreateProcess in windows #32404

Open
atetubou opened this issue Jun 3, 2019 · 11 comments
Open

os/exec: export thread id come from CreateProcess in windows #32404

atetubou opened this issue Jun 3, 2019 · 11 comments

Comments

@atetubou
Copy link
Contributor

@atetubou atetubou commented Jun 3, 2019

This is feature request.

On windows, we cannot get thread id of spawned process by os/exec package without using Tool Help Library.

But I want to use thread id returned from CreateProcess api.
Because I'd like to use Job Objects in our go application.
In our application, we need to create process with CREATE_SUSPENDED flag to assign job object before process starts.
But without thread id, it is difficult to get thread handle passed to ResumeThread for created process.

Can I add a filed tid to syscall.SysProcAttr, so that I can know thread id of created process?

@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Jun 3, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 3, 2019

What is the value of cmd.Process.Pid on Windows? It has some meaning, but I don't know if it is the value you are looking for.

@atetubou

This comment has been minimized.

Copy link
Contributor Author

@atetubou atetubou commented Jun 3, 2019

cmd.Process.Pid is id of process on Windows, not id of thread.
I'd like to get dwThreadId from PROCESS_INFORMATION instead of dwProcessId.

@maruel

This comment has been minimized.

Copy link
Contributor

@maruel maruel commented Jun 3, 2019

What @atetubou is asking is for the thread handle, not the thread id.
Right now, StartProcess() immediately closes the thread handle before returning to the client.
https://go.googlesource.com/go/+/refs/tags/go1.12.5/src/syscall/exec_windows.go#333

@atetubou

This comment has been minimized.

Copy link
Contributor Author

@atetubou atetubou commented Jun 3, 2019

@maruel I'm thinking export thread id instead of thread handle now.
Better to let users get thread handle from thread id because I don't want users to choose whether leaking thread handle or not.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 4, 2019

Change https://golang.org/cl/180398 mentions this issue: syscall: allow returning thread handle from StartProcess

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 4, 2019

On windows, we cannot get thread id of spawned process by os/exec package without using Tool Help Library.

What is wrong with using CreateToolhelp32Snapshot ? There is a runtime.TestNumCPU that does what you want. Why cannot you do the same?

Can I add a filed tid to syscall.SysProcAttr, so that I can know thread id of created process?

Adding just dwThreadId will not work, because it will become invalid the moment hThread is closed.

syscall.SysProcAttr is already complicated enough. And syscall.SysProcAttr at this moment is only used to pass data into CreateProcess. You propose syscall.SysProcAttr to be used to return data from CreateProcess. And whoever uses hThread must remember to closes it as soon as they are done with it, otherwise they will end up with leaked handles. And errors caused by leaked handles are always difficult to debug.

Is your use case worth complicating syscall.SysProcAttr?

Alex

@maruel

This comment has been minimized.

Copy link
Contributor

@maruel maruel commented Jun 4, 2019

If I were to propose on linux:

"On every process creation, we'll ask the OS to take a snapshot of every threads on the system, allocating a significant amount of kernel memory scaled to the active workload; more workload, the slower it is"

I think you would think it's a very bad idea that doesn't scale. I don't see why this would be more acceptable on Windows. That's my objection on CreateToolhelp32Snapshot.

Adding just dwThreadId will not work, because it will become invalid the moment hThread is closed.

This is plain untrue, I don't know why you think thread ID changes on Windows. You can use OpenThread by the thread id.

But OpenThread itself is problematic. In some case (for example with Chrome's Sandbox on Windows) or with CreateProcessAsUser, a process could be created in such way that its SECURITY_DESCRIPTOR inhibits opening a thread handle, while the thread returned by CreatreProcess/CreateProcessAsUser is usable. But the Go stdlib closes the handle with no option to opt out of this behavior. Finally, closing an handle just to open it back in a performance critical code path doesn't sound good design pattern.

So yes, it's worth it. A single Chrome compile is over 50000 process creation and is both highly I/O and CPU bound.

What @atetubou proposes is the less disruptive option given the current constraints, and also the most performant one.

@atetubou

This comment has been minimized.

Copy link
Contributor Author

@atetubou atetubou commented Jun 4, 2019

But OpenThread itself is problematic. In some case (for example with Chrome's Sandbox on Windows) or with CreateProcessAsUser, a process could be created in such way that its SECURITY_DESCRIPTOR inhibits opening a thread handle, while the thread returned by CreatreProcess/CreateProcessAsUser is usable.

But we don't call CreateProcess with such SECURITY_DESCRIPTOR here?

@maruel

This comment has been minimized.

Copy link
Contributor

@maruel maruel commented Jun 4, 2019

Not yet. We will once we implement lowering the process token integrity level, but that's a separate issue outside the scope of the issue here.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 6, 2019

I think you would think it's a very bad idea that doesn't scale. I don't see why this would be more acceptable on Windows. That's my objection on CreateToolhelp32Snapshot.

Maybe you are right, maybe you are wrong. I have no idea how expensive that scenario would be in your setup. But I would actually try and see.

This is plain untrue, I don't know why you think thread ID changes on Windows.

That is how I interpreted

dwThreadId

A value that can be used to identify a thread. The value is valid from the time the thread is created until all handles to the thread are closed and the thread object is freed; at this point, the identifier may be reused.

from https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/ns-processthreadsapi-process_information How can you be sure that all handles to the thread are not closed and the thread object is freed by the time you decide to use dwThreadId ?

What @atetubou proposes is the less disruptive option given the current constraints, and also the most performant one.

What you propose is complicated and unsafe new API that all users of syscall.SysProcAttr need to learn without actually using it. If performance is so important to you, you should just copy whatever code you need and adjust the copy as you see fit.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.