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

Use current STJ in HostModel and remove unnecessary audit suppressions #109852

Merged
merged 4 commits into from
Feb 4, 2025
Merged

Use current STJ in HostModel and remove unnecessary audit suppressions #109852

merged 4 commits into from
Feb 4, 2025

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Nov 15, 2024 •
edited
Loading

Fixes #108262 and follow-up work from 31e28ad
Blocked on dotnet/sdk#44042

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Nov 15, 2024
@ViktorHofer ViktorHofer mentioned this pull request Nov 15, 2024
Closed
@ViktorHofer ViktorHofer added blocked Issue/PR is blocked on something - see comments area-Infrastructure-installer and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Nov 15, 2024
@ericstj
Copy link
Member

ericstj commented Nov 15, 2024

I support the goal of this PR, I tried to do it before, but had to roll it back because of #108261

Be sure that you test those scenarios so that we don't break again. IIRC we didn't see the crossgen problems on PR it was only afterward.

ericstj
ericstj reviewed Nov 15, 2024
View reviewed changes
src/tasks/Directory.Build.targets
<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonToolsetVersion)" ExcludeAssets="Runtime" PrivateAssets="All" />
</ItemGroup>

<!-- Suppress System.Text.Json/8.0.4 advisory as desktop msbuild doesn't yet provide binding redirects for the non-vulnerable version (8.0.5). -->
Copy link
Member

Choose a reason for hiding this comment

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

Is this no longer the case for the VS versions we are using?

Copy link
Member Author

Choose a reason for hiding this comment

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

Still the case but I decided to apply the NU1903 NoWarn directly on the STJ PackageReference above so that this won't pop up again with a future vulnerability report for STJ.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Nov 15, 2024 •
edited
Loading

I support the goal of this PR, I tried to do it before, but had to roll it back because of #108261

I saw the exact same error when testing with vstest locally. I posted a comment in dotnet/sdk#44042 as that's the underlying reason why this doesn't work. When testing with xunit.console.dll, things work fine.

Be sure that you test those scenarios so that we don't break again. IIRC we didn't see the crossgen problems on PR it was only afterward.

Can you please elaborate on the crossgen problems?

@build-analysis build-analysis bot mentioned this pull request Nov 15, 2024
Open
3 tasks
This was referenced Feb 3, 2025
Open
Open
Open
@ViktorHofer
Copy link
Member Author

Internal validation build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2634518&view=results

@ViktorHofer
Copy link
Member Author

@ericstj please take a look. The change should be ready now. Find the internal validation build above.

ericstj
ericstj reviewed Feb 4, 2025
View reviewed changes
src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj
@@ -21,14 +21,9 @@
<ItemGroup>
<!-- SDK pins this to a lower version https://github.com/dotnet/sdk/issues/43325 -->
<PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataToolsetVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Is this one possible to replace too? I know we haven't had a CVE in this, but it would be nice if we could make all of HostModel's dependencies live.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #112134

ericstj
ericstj approved these changes Feb 4, 2025
View reviewed changes
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Love it. Just one "scope creep" comment which you could try to address or peel off into a new issue depending on what you think about it.

@ViktorHofer ViktorHofer mentioned this pull request Feb 4, 2025
Open
@ViktorHofer ViktorHofer merged commit d44116e into main Feb 4, 2025
157 of 160 checks passed
@ViktorHofer ViktorHofer deleted the UseCurrentSTJInHostModelAndRemoveAuditSuppressions branch February 4, 2025 17:29
@jkoritzinsky
Copy link
Member

If we're actually able to get HostModel using live assets, we can move it into the libraries partition. The main reason it lived outside of libraries was due to dependency problems and infrastructure issues.

grendello added a commit to grendello/runtime that referenced this pull request Feb 5, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
be6a3e7 
* main:
  JIT: Set PGO data inconsistent when flow disappears in cast expansion (dotnet#112147)
  [H/3] Fix handling H3_NO_ERROR (dotnet#112125)
  Change some workflows using `pull_request` to use `pull_request_target` instead (dotnet#112161)
  Annotate ConfiguredCancelableAsyncEnumerable T with allows ref struct and update extensions (dotnet#111953)
  Delete copy of performance pipelines in previous location (dotnet#112113)
  Optimize BigInteger.Divide (dotnet#96895)
  Use current STJ in HostModel and remove unnecessary audit suppressions (dotnet#109852)
  JIT: Unify handling of InstParam argument during inlining (dotnet#112119)
  Remove unneeded DiagnosticSource content (dotnet#112116)
  Improve compare-and-branch sequences produced by Emitter (dotnet#111797)
  Jit: Conditional Escape Analysis and Cloning (dotnet#111473)
  Re-enable HKDF-SHA3 on Azure Linux
  Remove fstream usage from corehost (dotnet#111859)
am11
am11 reviewed Feb 27, 2025
View reviewed changes
src/installer/managed/Microsoft.NET.HostModel/Microsoft.NET.HostModel.csproj
@@ -21,14 +21,9 @@
<ItemGroup>
<!-- SDK pins this to a lower version https://github.com/dotnet/sdk/issues/43325 -->
<PackageReference Include="System.Reflection.Metadata" Version="$(SystemReflectionMetadataToolsetVersion)" />
<!-- The SDK distributes the live version of Json we can't reference that https://github.com/dotnet/runtime/issues/108262 -->
<PackageReference Include="System.Text.Json" Version="$(SystemTextJsonToolsetVersion)" />
<PackageReference Include="System.Memory" Version="$(SystemMemoryVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

System.Memory was required by STJ, it can also be removed now that STJ is coming from ProjectReference (which brings its own live System.Memory, transitively).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@am11 am11 am11 left review comments

@ericstj ericstj ericstj approved these changes

@maraf maraf Awaiting requested review from maraf maraf is a code owner

@ilonatommy ilonatommy Awaiting requested review from ilonatommy ilonatommy is a code owner

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@pavelsavara pavelsavara Awaiting requested review from pavelsavara pavelsavara is a code owner

Assignees

@ViktorHofer ViktorHofer

Labels
area-Infrastructure-installer blocked Issue/PR is blocked on something - see comments
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HostModel does not reference the version of System.Text.Json that it runs on in the SDK
4 participants
@ViktorHofer @ericstj @jkoritzinsky @am11