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

Single-file apps should not use copy-on-write mapping of the .exe on windows. #59033

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Sep 13, 2021

Fixes: #55405 (App published as single file fails to run in Windows 10.0.17763 containers)

Customer Impact

Regression from .NET Core 2.1 and 3.1 causing failure when a single-file app runs in a Windows 10.0.17763 container.

Failed to map file. CreateFileMappingW(C:\app\app.exe) failed with error 5
Failure processing application bundle.
Failed to map bundle file [C:\app\app.runtimeconfig.json]

(This is a relatively old 2018 RS5 build, but it is an LTS so it will be around for a while)

Testing

Manually verified in an actual container. Regular Unit tests.

Risk

Very low.

We do not benefit from copy-on-write part of the mapping on Windows since we do not do in-situ json parsing due to UTF8/UTF16 differences. We basically request COW for consistency with Unix. Somehow it triggers unexpected "access denied" failure for superhost-style single-file apps on this particular build/container.

Since we do not do in-situ parsing on Windows, a read-only mapping is sufficient and we should request just what we need.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Sep 13, 2021

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

We do not benefit from copy-on-write part of the mapping on Windows, we do it for consistency with Unix. However it appears to be causing problems for single-file apps in some LTS versions of Windows containers.

Read-only mapping is sufficient on Windows and we should request just that.

UNDONE: I have not confirmed if this is the only problem in Windows containers + single-file.
Re: #55405

Author: VSadov
Assignees: -
Labels:

area-Single-File

Milestone: -

@agocke
Copy link
Member

agocke commented Sep 13, 2021

Is it confirmed that this fixes the problem?

@VSadov
Copy link
Member Author

VSadov commented Sep 14, 2021

@agocke

Is it confirmed that this fixes the problem?

At this moment I think it is a very likely cause for the failure. It is also a case where we ask for extra permissions from the OS while we do not need them. It may make sense to make this change regardless.

I have not confirmed yet whether it actually fixes the issue and whether this is the only reason for the failure. (otherwise it would not need to be in 6.0) - Just need to figure how to get a locally compiled app into the same container as in the repro and run it. There should be a way.

@VSadov
Copy link
Member Author

VSadov commented Sep 15, 2021

The repro turned out even more bizarre than reported. - The failure is observed only on the first execution of the app. If the app is launched again in the same container instance, it runs fine, although "please run the app twice" does not look like a a reasonable workaround, so it still looks bad.

I.E:

Microsoft Windows [Version 10.0.17763.2114]
(c) 2018 Microsoft Corporation. All rights reserved.

C:\app>app.exe
Failed to map file. CreateFileMappingW(C:\app\app.exe) failed with error 5
Failure processing application bundle.
Failed to map bundle file [C:\app\app.runtimeconfig.json]

C:\app>app.exe
Hello, World!

C:\app>

Anyways, I did confirm that with this fix the strange behavior is not triggered and the app runs fine on the first run (and subsequent runs too :).

After the fix:

Microsoft Windows [Version 10.0.17763.2114]
(c) 2018 Microsoft Corporation. All rights reserved.

C:\app>app.exe
Hello, World!

C:\app>app.exe
Hello, World!

C:\app>app.exe
Hello, World!

C:\app>app.exe
Hello, World!

C:\app>app.exe
Hello, World!

C:\app>

@VSadov VSadov marked this pull request as ready for review September 15, 2021 00:22
@VSadov VSadov changed the title Single-file apps should not use copy-on-write mapping of the .exe on windows. [release/6.0] Single-file apps should not use copy-on-write mapping of the .exe on windows. Sep 15, 2021
@agocke agocke changed the base branch from release/6.0 to main September 16, 2021 08:14
@agocke agocke self-assigned this Sep 16, 2021
Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@agocke agocke changed the title [release/6.0] Single-file apps should not use copy-on-write mapping of the .exe on windows. Single-file apps should not use copy-on-write mapping of the .exe on windows. Sep 16, 2021
@agocke
Copy link
Member

agocke commented Sep 16, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1240823258

Comment on lines +112 to +113
// Since we can't use in-situ parsing on Windows, as JSON data is encoded in
// UTF-8 and the host expects wide strings.
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 part of the comment related to the code below on line 115 or above line 88?

Copy link
Member

Choose a reason for hiding this comment

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

This is about 115 -- the RapidJson parser used in json_parser.cpp modifies the buffer on Linux, but not on Windows, so the CoW permissions are not useful on Windows.

@agocke agocke merged commit f1425cf into dotnet:main Sep 16, 2021
@VSadov VSadov deleted the nocow branch September 16, 2021 17:04
@VSadov
Copy link
Member Author

VSadov commented Sep 16, 2021

Thanks!!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App published as single file fails to run in Windows 10.0.17763 containers
4 participants