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

Only inherit the handles that are really needed. #887

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

autoantwort
Copy link
Contributor

Currently all handles are inherited to child processes. For more information see https://discord.com/channels/400588936151433218/687365466422902841/1072485165491814430

@autoantwort autoantwort force-pushed the feature/only-inherit-pipes branch 2 times, most recently from 26e1619 to da62c51 Compare February 7, 2023 14:05
@autoantwort autoantwort force-pushed the feature/only-inherit-pipes branch from da62c51 to db3c030 Compare February 7, 2023 14:35
// Ensure that only the write handle to STDOUT and the read handle to STDIN are inherited.
// from https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873
SIZE_T size = 0;
if (InitializeProcThreadAttributeList(nullptr, 1, 0, &size) || GetLastError() != ERROR_INSUFFICIENT_BUFFER)
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to see an RAII type for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for the clean up or for everything? So should a failure in the constructor (calls InitializeProcThreadAttributeList) throw an exception?

Copy link
Member

@BillyONeal BillyONeal Feb 8, 2023

Choose a reason for hiding this comment

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

I wish it were for the pipes too but that's preexisting so won't ask you to go there. I meant something like:

struct ProcAttributeList {
    static ExpectedL<ProcAttributeList> create(DWORD dwAttributeCount);
    ExpectedL<Unit> update_attribute(DWORD_PTR Attribute, PVOID lpValue, SIZE_T cbSize);
    ~ProcAttributeList();
    LPPROC_THREAD_ATTRIBUTE_LIST get() const noexcept { return reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(buffer.data()); }

    ProcAttributeList(const ProcAttributeList&) = delete;
    ProcAttributeList& operator=(const ProcAttributeList&) = delete;
private:
    ProcAttributeList(/* *** */);
    std::vector<unsigned char> buffer;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have done this in a3bbb30

@BillyONeal BillyONeal merged commit f70f216 into microsoft:main Feb 8, 2023
@BillyONeal
Copy link
Member

Thanks for the fix!

@autoantwort autoantwort deleted the feature/only-inherit-pipes branch February 8, 2023 19:10
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.

2 participants