Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Protect against circular type forwarders #806

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KirillOsenkov
Copy link

If we have two assemblies with type forwarders that point to each other, we enter an infinite loop and a stack overflow.

This breaks the cycle by detecting reentrancy.

Fixes #706

If we have two assemblies with type forwarders that point to each other, we enter an infinite loop and a stack overflow.

This breaks the cycle by detecting reentrancy.

Fixes jbevain#706
@KirillOsenkov KirillOsenkov force-pushed the dev/kirillo/reentrancy branch from 655d831 to 5ad5672 Compare October 15, 2023 22:40
@KirillOsenkov
Copy link
Author

@jbevain I even added a test!

@jonlouie
Copy link

@jbevain What can community contributors do to get this PR merged and released? This resolves a bug that my team encountered.

@KirillOsenkov
Copy link
Author

FYI @jbevain adding the boolean field is free because of alignment padding:

image

@KirillOsenkov
Copy link
Author

KirillOsenkov commented Nov 20, 2024

Unfortunately it's not free for 32-bit (the size goes from 32 to 36 bytes. But there are not many exported types per assembly, so it's honestly negligible.

Type layout for 'ExportedType'
Size: 36 bytes. Paddings: 3 bytes (%8 of empty space)
|==============================================|
| Object Header (4 bytes)                      |
|----------------------------------------------|
| Method Table Ptr (4 bytes)                   |
|==============================================|
|   0-3: String namespace (4 bytes)            |
|----------------------------------------------|
|   4-7: String name (4 bytes)                 |
|----------------------------------------------|
|  8-11: IMetadataScope scope (4 bytes)        |
|----------------------------------------------|
| 12-15: ModuleDefinition module (4 bytes)     |
|----------------------------------------------|
| 16-19: ExportedType declaring_type (4 bytes) |
|----------------------------------------------|
| 20-23: UInt32 attributes (4 bytes)           |
|----------------------------------------------|
| 24-27: Int32 identifier (4 bytes)            |
|----------------------------------------------|
|    28: Boolean reentrancyGuard (1 byte)      |
|----------------------------------------------|
| 29-31: padding (3 bytes)                     |
|----------------------------------------------|
| 32-35: MetadataToken token (4 bytes)         |
| |===============================|            |
| |   0-3: UInt32 token (4 bytes) |            |
| |===============================|            |
|==============================================|

@cwensley
Copy link

We've been running into these same stack overflow issues quite a bit but intermittently and not related to circular type forwarders (from what I can tell). I have confirmed this fixes our issues as well, so it'd be nice to have it included in an official release.

@jbevain
Copy link
Owner

jbevain commented Feb 5, 2025

@KirillOsenkov just move the field with the other fields and this is good to go :)

@KirillOsenkov
Copy link
Author

It's happening.gif

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Defend against cyclical Type Forwarders?
4 participants