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

Delete IL verifier from the JIT #32648

Closed
jkotas opened this issue Feb 21, 2020 · 14 comments
Closed

Delete IL verifier from the JIT #32648

jkotas opened this issue Feb 21, 2020 · 14 comments
Assignees
@amanasifkhalid
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone
Future

Comments

@jkotas
Copy link
Member

jkotas commented Feb 21, 2020 •
edited by BruceForstall
Loading

IL verifier is no longer needed in the JIT. Delete tiVerificationNeeded and all code under it.

More context: #32521 (comment)

category:implementation
theme:verification
skill-level:beginner
cost:small
impact:small
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Feb 21, 2020
@GrabYourPitchforks
Copy link
Member

Is there still a scenario where it makes sense for peverify to exist as a stand-alone tool? I'm struggling to think of one aside from checking whether your code contains any dangerous constructs, but even then its incomplete.

@jkotas
Copy link
Member Author

jkotas commented Feb 21, 2020

Is there still a scenario where it makes sense for peverify to exist as a stand-alone tool?

Yes, there is. It is tracked by #13827

Note that the RyuJIT IL verifier is not the IL verifier used by PEVerify. RyuJIT IL verifier just gives yes / no answer, without producing the good diagnostic for telling you what is wrong. PEVerify had a completely different implementation of IL verifier.

@jashook
Copy link
Contributor

jashook commented Feb 25, 2020

/cc @BruceForstall @erozenfeld

@BruceForstall BruceForstall added this to the Future milestone Apr 4, 2020
@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Apr 4, 2020
@BruceForstall BruceForstall modified the milestones: Future, 5.0 Apr 4, 2020
@AndyAyersMS
Copy link
Member

Might be some small throughput benefit, so let's keep in 5.0.

@BruceForstall
Copy link
Member

I think it's too late in the product cycle for us to do this. Moving to Future.

@BruceForstall BruceForstall modified the milestones: 5.0.0, Future Jul 1, 2020
sandreenko pushed a commit to sandreenko/runtime that referenced this issue Sep 8, 2020
Add GetStructHnd to LclVarDsc.
9866976 
A small contirubtion to dotnet#32648.
@sandreenko sandreenko mentioned this issue Sep 15, 2020
Merged
sandreenko pushed a commit to sandreenko/runtime that referenced this issue Sep 18, 2020
Add GetStructHnd to LclVarDsc.
1f66b1d 
A small contirubtion to dotnet#32648.
sandreenko pushed a commit that referenced this issue Sep 19, 2020
A small ref PR in preparation for arm64 apple work. (#42270)
1905238 
* Mark some getters as const to avoid build errors.

* use `gtGetStructHandle` if `NO_CLASS_HANDLE` means an error.

* Add `GetStructHnd` to `LclVarDsc`.

A small contirubtion to #32648.

* additional asserts
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
BruceForstall added a commit to BruceForstall/runtime that referenced this issue Mar 21, 2021
@BruceForstall
First pass removing IL verifier from JIT
f904418 
dotnet#32648
@BruceForstall BruceForstall mentioned this issue Mar 21, 2021
Closed
@SingleAccretion SingleAccretion mentioned this issue Dec 29, 2021
Open
@masonwheeler
Copy link
Contributor

At first glance this kind of feels like a bad idea. Having a verifier at runtime has some very real benefits, particularly in the realm of security. Remember when Java was new and malicious coders started using hand-crafted bytecode exploits to break its security, and they fixed it by adding a runtime verifier? If the IL verifier is removed from the JIT, is there anything to stop that story from replaying itself?

@alexrp
Copy link
Contributor

alexrp commented Dec 29, 2021

Sandboxing has not been a supported scenario in .NET for a very, very long time. It's already trivial to break memory safety in countless ways.

@JulieLeeMSFT
Copy link
Member

cc @TIHan .

@TIHan
Copy link
Contributor

TIHan commented Jan 26, 2022

Will take a look.

@TIHan TIHan self-assigned this Jan 26, 2022
@TIHan TIHan mentioned this issue Jan 26, 2022
Merged
@TIHan TIHan linked a pull request Jan 26, 2022 that will close this issue
Removing tiVerificationNeeded in the JIT #64356
Merged
@JulieLeeMSFT JulieLeeMSFT modified the milestones: Future, 7.0.0 Jan 27, 2022
@TIHan TIHan removed a link to a pull request Jan 27, 2022
Removing tiVerificationNeeded in the JIT #64356
Merged
@TIHan
Copy link
Contributor

TIHan commented Jan 28, 2022

This PR #64356 will remove the use of tiVerificationNeeded and addVerifyFlag as a first pass of removal.

Also mentioning this issue, #64399, as it is related to the overall deletion of the IL verifier.

@TIHan TIHan mentioned this issue Feb 4, 2022
Closed
14 tasks
jkotas added a commit to jkotas/runtime that referenced this issue Mar 14, 2022
@jkotas
Delete IL verifier support in the VM
c4a1c0c 
Contributes to dotnet#32648
@jkotas jkotas mentioned this issue Mar 14, 2022
Merged
jkotas added a commit that referenced this issue Mar 20, 2022
@jkotas
Delete IL verifier support in the VM (#66616)
c3fa765 
Contributes to #32648
radekdoulik pushed a commit to radekdoulik/runtime that referenced this issue Mar 30, 2022
@jkotas @radekdoulik
Delete IL verifier support in the VM (dotnet#66616)
8de6da7 
Contributes to dotnet#32648
@JulieLeeMSFT JulieLeeMSFT modified the milestones: 7.0.0, 8.0.0 Jun 6, 2022
@TIHan TIHan removed the JitUntriaged CLR JIT issues needing additional triage label Jun 22, 2022
@TIHan TIHan modified the milestones: 8.0.0, Future Jul 5, 2022
@SingleAccretion SingleAccretion mentioned this issue Jul 24, 2022
Merged
@kant2002
Copy link
Contributor

@SingleAccretion just curious, is there something left from this task?

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Sep 22, 2022 •
edited
Loading

Yeah, there is still the task of deleting the separate TI_ "type system" from the Jit.

Edit: and other vestiges like verConvertBBToThrowVerificationException, etc.

@SingleAccretion SingleAccretion mentioned this issue May 12, 2023
Merged
@JulieLeeMSFT JulieLeeMSFT assigned amanasifkhalid and unassigned TIHan Oct 15, 2024
@JulieLeeMSFT
Copy link
Member

Reassigned to @amanasifkhalid.

@amanasifkhalid amanasifkhalid mentioned this issue Oct 17, 2024
Merged
@amanasifkhalid
Copy link
Member

I believe #108996 finished this issue off, though feel free to re-open this if I'm wrong.

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

@amanasifkhalid amanasifkhalid

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
Future
Development

No branches or pull requests