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.

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

Fix StatInterop to include new signature #26916

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Aug 1, 2022

The LStat native method was updated to include a new field, but the SDK copy wasn't updated. Adding the new field to fix interop issues and crashes.

Fix dotnet/runtime#72308

I verified this fixes the issue manually. I'm not sure how to test this in CI. @dsplaisted - any thoughts?

The LStat native method was updated to include a new field, but the SDK copy wasn't updated. Adding the new field to fix interop issues and crashes.

Fix dotnet/runtime#72308
@dsplaisted
Copy link
Member

I think this is a great reason why we don't recommend people to p/invoke into our System.*.Native shims. They are not stable ABI.

Is there a better way to do this (call lstat, it looks like) that is less likely to break?

@eerhardt
Copy link
Member Author

eerhardt commented Aug 1, 2022

Is there a better way to do this (call lstat, it looks like) that is less likely to break?

Unfortunately the best way I know of currently is to use https://github.com/mono/mono.posix, but that doesn't work on alpine according to mono/mono.posix#18.

The issue with p/invoking directly into libc's lstat is that it moves around based on which unix you are on.

In .NET 7, we added support for getting Unix File Modes with dotnet/runtime#69980. Using that would eliminate all but one usage: getting the owner of a file. We don't currently have that API in .NET. We have a few issues that touch on it like dotnet/runtime#928 and dotnet/runtime#17540. If dotnet/sdk really needs to get the owner of the file/folder, we should consider adding these APIs to .NET.

@eerhardt
Copy link
Member Author

eerhardt commented Aug 1, 2022

I think this is a great reason why we don't recommend people to p/invoke into our System.*.Native shims. They are not stable ABI.

I've opened #26919 to track the underlying issue here.

@eerhardt
Copy link
Member Author

eerhardt commented Aug 2, 2022

@dsplaisted - any more thoughts here? Should we merge this to unblock the arm64 scenarios?

@ivdiazsa
Copy link
Member

ivdiazsa commented Aug 3, 2022

@eerhardt This looks great! I was wondering though, should we open another tracking issue for the missing directory creation like pointed here dotnet/runtime#72308 (comment) ?

@eerhardt
Copy link
Member Author

eerhardt commented Aug 3, 2022

I was wondering though, should we open another tracking issue for the missing directory creation like pointed here dotnet/runtime#72308 (comment) ?

Feel free to open it if it is causing problems. Just remember to include repro steps.

@mangod9
Copy link
Member

mangod9 commented Aug 3, 2022

@dsplaisted is this good to merge? Would the incorrect layout explain possible memory corruption we were observing?

@dsplaisted
Copy link
Member

@dsplaisted is this good to merge? Would the incorrect layout explain possible memory corruption we were observing?

I'm not an expert on native interop, but I was expecting that yes, this would explain the native crashes from dotnet/runtime#72308

@dsplaisted dsplaisted merged commit ed22bd1 into dotnet:main Aug 5, 2022
@eerhardt eerhardt deleted the FixLStat branch August 8, 2022 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sudo dotnet workload --help segfaults on Apple M1 in .NET 7 Preview 6
5 participants