Skip to content

Terminate the loop in PsUtils.GetMainModule() which was infinite in case there was no main module. #6358

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

Merged
adityapatwardhan merged 18 commits into from
Apr 3, 2018
Merged

Terminate the loop in PsUtils.GetMainModule() which was infinite in case there was no main module. #6358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d71760a
Terminate the loop in GetMainModule if main module is null.
kalgiz Mar 10, 2018
b457c01
Code cleanup
kalgiz Mar 12, 2018
5a250bb
[Feature] Further clean up the code
daxian-dbw Mar 12, 2018
80bff4f
Merge branch 'master' of https://github.com/PowerShell/PowerShell int…
kalgiz Mar 20, 2018
5990167
[Feature] Run tests with FileVersionInfo also on non-windows platform…
kalgiz Mar 20, 2018
16fd838
[Feature] Tests correction for Get-process -FileversionInfo command.
kalgiz Mar 21, 2018
9483699
[Feature] Get-process -FileVersionInfo parameter tests correction.
kalgiz Mar 21, 2018
33cacc1
[Feature] Get-Process test fix.
kalgiz Mar 21, 2018
f9a2d56
Merge branch 'master' of https://github.com/PowerShell/PowerShell int…
kalgiz Mar 22, 2018
3f5556d
Merge branch 'master' of https://github.com/kalgiz/PowerShell into ge…
kalgiz Mar 23, 2018
494e0f0
Merge branch 'master' of https://github.com/kalgiz/PowerShell into ge…
kalgiz Mar 23, 2018
65fcf99
Merge branch 'get-process-fileversioninfo-bug' of https://github.com/…
kalgiz Mar 23, 2018
fd351d8
[Feature] Get-Process test correction.
kalgiz Mar 23, 2018
9182513
[Feature] Tests correction.
kalgiz Mar 23, 2018
f07dd26
[Feature] Tests correction.
kalgiz Mar 23, 2018
3f6b1bd
[Feature] Test correction.
kalgiz Mar 23, 2018
982a314
[Feature] Merge branch 'master' of https://github.com/PowerShell/Powe…
kalgiz Mar 28, 2018
4e350e4
[Feature] Merge branch 'get-process-fileversioninfo-bug' of https://g…
kalgiz Mar 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,10 @@ protected override void ProcessRecord()
//if fileversion of each process is to be displayed
try
{
WriteObject(PsUtils.GetMainModule(process).FileVersionInfo, true);
ProcessModule mainModule = PsUtils.GetMainModule(process);
if (mainModule != null) {
Copy link
Member

@daxian-dbw daxian-dbw Mar 12, 2018

Choose a reason for hiding this comment

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

When mainModule == null, is the expected behavior to be writing out nothing or throwing an error?
On windows, the current behavior is to throw error when the FileInfoVersion is not accessible.

@JamesWTruher Your insight would be helpful here.

Copy link
Collaborator

@iSazonov iSazonov Mar 13, 2018

Choose a reason for hiding this comment

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

Related comment from CoreFX https://github.com/dotnet/corefx/blob/f90aa6665a59dc6caaff995a79fb852d21fe6f68/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L176

Copy link
Collaborator

@JamesWTruher JamesWTruher Mar 20, 2018

Choose a reason for hiding this comment

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

it's also possible on non-windows platforms that mainmodule could be null due to the underlying corefx implementation (ex: process id 0). Throwing an error doesn't seem reasonable.

WriteObject(mainModule.FileVersionInfo, true);
}
}
catch (InvalidOperationException exception)
{
Expand Down
7 changes: 2 additions & 5 deletions src/System.Management.Automation/utils/PsUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,12 @@ internal static class PsUtils
internal static ProcessModule GetMainModule(Process targetProcess)
{
int caughtCount = 0;
ProcessModule mainModule = null;

while (mainModule == null)
while (true)
{
try
{
mainModule = targetProcess.MainModule;
return targetProcess.MainModule;
}
catch (System.ComponentModel.Win32Exception e)
Copy link
Collaborator

@iSazonov iSazonov Mar 13, 2018

Choose a reason for hiding this comment

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

Should we catch InvalidOperationException?
https://msdn.microsoft.com/en-us/library/system.diagnostics.process.mainmodule%28v=vs.110%29.aspx

Copy link
Contributor Author

@kalgiz kalgiz Mar 19, 2018

Choose a reason for hiding this comment

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

@daxian-dbw what do you think?

Copy link
Member

@daxian-dbw daxian-dbw Mar 19, 2018

Choose a reason for hiding this comment

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

I don't think we should catch InvalidOperationException. It seems to me we catch Win32Exception only to work around the flaw described in the summary.

Copy link
Collaborator

@iSazonov iSazonov Mar 20, 2018

Choose a reason for hiding this comment

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

Why we should wait if process already exit? I'd re-throw immediately.

Copy link
Member

@daxian-dbw daxian-dbw Mar 20, 2018

Choose a reason for hiding this comment

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

InvalidOperationException will be thrown when process already exists, but we don't catch InvalidOperationException and thus no waiting in that case.

Copy link
Collaborator

@iSazonov iSazonov Mar 20, 2018

Choose a reason for hiding this comment

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

Thanks for clarify!

{
Expand All @@ -81,8 +80,6 @@ internal static ProcessModule GetMainModule(Process targetProcess)
throw;
}
}

return mainModule;
}

// Cache of the current process' parentId
Expand Down
20 changes: 18 additions & 2 deletions test/powershell/Modules/Microsoft.PowerShell.Management/Get-Process.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,25 @@ Describe "Get-Process for admin" -Tags @('CI', 'RequireAdminOnWindows') {
}
}

It "Should support -FileVersionInfo" -Skip:(!$IsWindows) {
Copy link
Member

@SteveL-MSFT SteveL-MSFT Mar 20, 2018

Choose a reason for hiding this comment

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

This test is wrong for non-Windows. If you run the Get-Process line below on non-Windows, you'll see that FileVersion is $null. "anything" -match $null returns $true. This test for correct FileVersion being returned will only work on Windows. Perhaps we can modify this test to work on both Windows and non-Windows:

It "Should support -FileVersionInfo" {
  $pwshVersion = ...
  if ($IsWindows) {
     $pwshVersion | Should -BeExactly $PSVersionTable.PSVersion
 } else {
     $pwshVersion | Should -BeNullOrEmpty
 }
}

Copy link
Contributor Author

@kalgiz kalgiz Mar 21, 2018

Choose a reason for hiding this comment

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

Do we know that process' FileVersion always would be null or non windows platform?

Copy link
Member

@SteveL-MSFT SteveL-MSFT Mar 22, 2018

Choose a reason for hiding this comment

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

Yes, the FileVersion information is stored as resources in PE file format on Windows whereas Linux uses ELF format which is not compatible with this extended file information.

It "Should support -FileVersionInfo" {
$pwshVersion = Get-Process -Id $pid -FileVersionInfo
$PSVersionTable.PSVersion | Should -Match $pwshVersion.FileVersion
if ($IsWindows) {
$PSVersionTable.PSVersion | Should -MatchExactly $pwshVersion.FileVersion
$gitCommitId = $PSVersionTable.GitCommitId
if ($gitCommitId.StartsWith("v")) { $gitCommitId = $gitCommitId.Substring(1) }
$pwshVersion.ProductVersion.Replace("-dirty","") | Should -BeExactly $gitCommitId
} else {
$pwshVersion.FileVersion | Should -BeNullOrEmpty
}
}

It "Run with parameter -FileVersionInfo should not hang on non Windows platform also when process' main module is null." -Skip:$IsWindows {
# Main module for idle process can be null on non-Windows platforms
{ $pwshVersion = Get-Process -Id 0 -FileVersionInfo -ErrorAction Stop } | Should -Not -Throw
}

It "Run with parameter -FileVersionInfo for idle process should throw on Windows." -Skip:(!$IsWindows) {
{ $pwshVersion = Get-Process -Id 0 -FileVersionInfo -ErrorAction Stop } | Should -Throw -ErrorId "CouldNotEnumerateFileVer,Microsoft.PowerShell.Commands.GetProcessCommand"
}
}

Expand Down