Aggregate adjacent memory sizes regardless of r+x

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!

[am11]

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When adjacent memory ranges of same module differ by permission, the
line should not be skipped due to the lack of readability/executability
flags.

Author:	am11
Assignees:	-
Labels:	area-System.Diagnostics.Process
Milestone:	-

[am11]

cc @stephentoub
After removing the transport type ParsedMapsModule and directly using ProcessModule{Collection} in procfs methods, I had to make another split to the partial class to keep the code performant. Interop.ProcFsStat.cs is also compiled by Common.Tests.csproj (TryParseStatusFile() is used in procfsTests.cs), where ProcessModuleCollection and ProcessModule are readonly. Other option was to use InternalsVisibleToAttribute, which I think we are generally avoiding. Please let me know if there is a better way. :)

Tested with dotnet console template.

e.g. before:

currentProcessModules[i].FileName: /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 73728

after (extracted the built tarball from artifacts/packages/Shipping directory and copied runtime/.dotnet/sdk over):

currentProcessModules[i].FileName: /root/.dotnet_pr/shared/Microsoft.NETCore.App/5.0.0-rc.2.20475.5/libSystem.Native.so
currentProcessModules[i].ModuleMemorySize: 2174976

[eiriktsarpalis]

Would it be possible to write a test that verifies the new behaviour? Presumably there would no longer be any duplicates in the process modules for the current process?

[am11]

Currently we have a test validating that these properties do not throw:

runtime/src/libraries/System.Diagnostics.Process/tests/ProcessTests.cs

Lines 624 to 627 in 4a743ac

	// Just make sure these don't throw
	IntPtrbaseAddr=pModule.BaseAddress;
	IntPtrentryAddr=pModule.EntryPointAddress;
	intmemSize=pModule.ModuleMemorySize;

Also there is a test which asserts that getting modules multiple times subsequently result in the same modules.
Is there anything else we could validate?

[eiriktsarpalis]

Is there anything else we could validate?

What issue is the PR meant to be addressing? Presumably adjacent regions with different permission sets would be reported as separate modules? Looking at the maps file for a .NET process in my machine I see this:

7efc48980000-7efc48cea000 r-xp 00000000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48cea000-7efc48ceb000 rwxp 0036a000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48ceb000-7efc48f23000 r-xp 0036b000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f23000-7efc48f24000 r--p 005a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48f24000-7efc48fff000 r-xp 005a4000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc48fff000-7efc49000000 ---p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49000000-7efc49024000 r--p 0067f000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so
7efc49024000-7efc4903c000 rw-p 006a3000 08:10 9953 /usr/share/dotnet/shared/Microsoft.NETCore.App/5.0.0/libcoreclr.so

Would this be reported differently after your changes? Or am I misunderstanding something?

[am11]

What issue is the PR meant to be addressing?

It was based on this discussion type thread: #45180.

Would this be reported differently after your changes?

From the example, this PR would only affect the value of netCoreModule.ModuleMemorySize as follow:

Before: 5910528
(7efc48f23000 – 7efc48980000 = 5A3000)

After: 7061504
(7efc4903c000 - 7efc48980000 = 6BC000)

Before we were stopping at the row without r and x permission. PR continues to sum all consecutive nodes of the same module (unless end address of previous and start address of current mismatch).

[eiriktsarpalis]

Ok, so the issue concerned reporting incorrect sizes? Thanks for clarifying.

[am11]

Installer failure Text file busy is #42101.

[adamsitnik]

@am11 is there any chance that you could split the PR into two commits: the actual fix and the refactoring? It would make reviewing much easier for us. Thank you!

[eiriktsarpalis]

@adamsitnik has suggested we create a unit test that validates the new parsing logic using a fixed maps file input. We would need to include the implementation source files in the test project given that they are internal.

[am11]

iOS simulator launch failure is #45181.

[tmds]

Thanks @am11 for fixing these issues!
@adamsitnik@eiriktsarpalis this lgtm, I leave further review to you.

[eiriktsarpalis]

Nit: I'm finding the parsing method hard to follow, mainly because it is cobbling together line parsing concerns with process module parsing concerns. I took the liberty of refactoring your code in a branch of my own, kindly consider including something like that.

[am11]

@eiriktsarpalis, thanks a lot. Based on:

✔️ DO use the prefix "Try" and Boolean return type for methods implementing this pattern.

from https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance#try-parse-pattern, I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better. Other than that it looks good. I will cherry-pick and adapt. :)

[eiriktsarpalis]

I have also found non-bool returning method with Try prefix in name a bit odd, so I would like void CommitCurrentModule better.

Agreed. It's internal as well so won't die on that hill :-)

[eiriktsarpalis]

Thanks!