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

Fix 32-bit process module enumeration on 64-bit Windows (#70370) #71218

Merged
merged 4 commits into from
Jun 24, 2022
Merged

Fix 32-bit process module enumeration on 64-bit Windows (#70370) #71218

merged 4 commits into from
Jun 24, 2022

Conversation

just-ero
Copy link
Contributor

Fixes the faulty Process.Modules behavior described in #70370.

  • Deleted ~\Kernel32\Interop.EnumProcessModules.cs and created ~\Kernel32\Interop.EnumProcessModulesEx.cs in its stead
  • Created Kernel32.Interop.LIST_MODULES_ALL in ~\Kernel32\Interop.Constants.cs
  • Changed NtProcessManager.GetModules and NtProcessManager.EnumProcessModulesUntilSuccess to use Interop.Kernel32.EnumProcessModulesEx instead of the previous Interop.Kernel32.EnumProcessModules

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 23, 2022
@ghost
Copy link

ghost commented Jun 23, 2022

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes the faulty Process.Modules behavior described in #70370.

  • Deleted ~\Kernel32\Interop.EnumProcessModules.cs and created ~\Kernel32\Interop.EnumProcessModulesEx.cs in its stead
  • Created Kernel32.Interop.LIST_MODULES_ALL in ~\Kernel32\Interop.Constants.cs
  • Changed NtProcessManager.GetModules and NtProcessManager.EnumProcessModulesUntilSuccess to use Interop.Kernel32.EnumProcessModulesEx instead of the previous Interop.Kernel32.EnumProcessModules
Author: just-ero
Assignees: -
Labels:

area-System.Diagnostics.Process
Milestone: -

@dnfadmin
Copy link

dnfadmin commented Jun 23, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

jkotas
jkotas approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas @stephentoub
Update src/libraries/Common/src/Interop/Windows/Kernel32/Interop.Enum…
683a4e8 
…ProcessModulesEx.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@jkotas
Copy link
Member

jkotas commented Jun 23, 2022

Test failure is #71233

adamsitnik
adamsitnik approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

Looks good to me, I am impressed that you were able to provide the fix so quickly!!

Big thanks @just-ero !

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs
@@ -221,15 +221,15 @@ private static ProcessModuleCollection GetModules(int processId, bool firstModul
}
}

private static void EnumProcessModulesUntilSuccess(SafeProcessHandle processHandle, IntPtr[]? modules, int size, out int needed)
private static void EnumProcessModulesUntilSuccess(SafeProcessHandle processHandle, IntPtr[]? modules, int size, out int needed, int filterFlag)
Copy link
Member

Choose a reason for hiding this comment

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

nit: if Interop.Kernel32.LIST_MODULES_ALL is the only value we use, you could avoid adding the new parameter to EnumProcessModulesUntilSuccess and just always pass Interop.Kernel32.LIST_MODULES_ALL to EnumProcessModulesEx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't wanna break any of the existing patterns.

@adamsitnik adamsitnik added this to the 7.0.0 milestone Jun 24, 2022
@adamsitnik adamsitnik self-assigned this Jun 24, 2022
@adamsitnik adamsitnik merged commit 911da68 into dotnet:main Jun 24, 2022
@adamsitnik adamsitnik mentioned this pull request Jun 24, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@adamsitnik adamsitnik adamsitnik approved these changes

@jkotas jkotas jkotas approved these changes

Assignees

@adamsitnik adamsitnik

Labels
area-System.Diagnostics.Process community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@just-ero @dnfadmin @jkotas @stephentoub @adamsitnik