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

Add winpty_get_console_process_list #130

Merged
merged 3 commits into from
Oct 8, 2017

Conversation

the-ress
Copy link
Contributor

For a better implementation of microsoft/vscode#30152 and microsoft/vscode#29926 we need a list of processes attached to the console (instead of using a full process tree).

GetConsoleProcessList needs to be called from a process attached to the console. Since winpty-agent is already attached to it, it seems to be a good place to call it from.

@rprichard
Copy link
Owner

I'll try to get to this in a week or two.

@Tyriar
Copy link

Tyriar commented Oct 2, 2017

@rprichard friendly ping 😃

@rprichard
Copy link
Owner

Looking at this code:

    auto processCount = GetConsoleProcessList(&processList[0], processList.size());
    if (processList.size() < processCount) {
        processList.resize(processCount);
        processCount = GetConsoleProcessList(&processList[0], processList.size());
    }

The process list can change while we're trying to read it, so I'd prefer that
the code loop while the processList vector was too small. Maybe on each time
through the loop, resize the vector to something like, "the greater of processCount * 2
and processCount". Multiplying by two caps the number of loop iterations. processCount * 2 could overflow, and if it does, then using
processCount will ensure that std::bad_alloc is raised (and turned into an
out-of-memory error at the winpty C API).

Otherwise, I'm happy with the PR.

Tyriar added a commit to microsoft/node-pty that referenced this pull request Oct 3, 2017
@the-ress
Copy link
Contributor Author

the-ress commented Oct 3, 2017

Thanks, I didn't think of that. It should be fixed now.

while (processList.size() < processCount) {
// Multiplying by two caps the number of iterations
const int newSize = processList.size() * 2;
if (newSize <= processList.size()) { // Ensure we fail when new size overflows
Copy link
Owner

Choose a reason for hiding this comment

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

If this code were to overflow, processList.size() would be 0x40000000, and newSize would be -0x80000000 (i.e. INT_MIN). newSize would be greater than processList.size() because it would be converted from int to size_t for the comparison.

I'd prefer to replace "const int newSize = ... ; ... if { ... }" with:

const auto newSize = std::max<DWORD>(processList.size() * 2, processCount);

If you do, also add #include <algorithm> to the top.

Overflow shouldn't ever happen. If it somehow does, then there is one code path handling it -- std::vector::resize throws std::bad_alloc.

@the-ress
Copy link
Contributor Author

the-ress commented Oct 7, 2017

I used std::max as you said...

@rprichard rprichard merged commit 850661d into rprichard:master Oct 8, 2017
@rprichard
Copy link
Owner

ok, thanks!

@the-ress the-ress deleted the get-console-process-list branch October 8, 2017 07:16
@Tyriar
Copy link

Tyriar commented Oct 10, 2017

🎉

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