SingleFile bundles: Ensure extraction mappings are closed on Windows. #2272

swaroop-sridhar · 2020-01-28T03:12:17Z

When running a single-file app, the AppHost mmap()s itself in order to read its contents and extract the embedded contents.

The Apphost must always map its contents in order to read the headers, but doesn't always extract the contents, because previously extracted files are re-used when available.

In the case where apphost doesn't extract, currently, the file mapping isn't immediately closed on Windows. This prevents the app from being renamed while running -- an idiom used while updating the app in-place.

This change fixes the problem by closing the open file-map.

Fixes #1260

jkotas · 2020-01-28T03:21:49Z

src/installer/test/Assets/TestProjects/AppWithWait/Program.cs

@@ -0,0 +1,16 @@
+using System;


License header?

There's no License header for any of the test assets in https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects

So, I kept the same pattern.

Can you please fix that in a followup PR? Or at least create an issue for it.

OK I'll fix it for all the projects in a subsequent PR.

jkotas · 2020-01-28T03:23:09Z

src/installer/corehost/common/pal.windows.cpp

@@ -109,7 +109,6 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t &length)
    if (map == NULL)


Suggested change

if (map == NULL)

if (address == NULL)

jkotas · 2020-01-28T03:24:34Z

src/installer/corehost/common/pal.windows.cpp

@@ -96,11 +96,11 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t &length)
    length = (size_t)fileSize.QuadPart;

    HANDLE map = CreateFileMappingW(file, NULL, PAGE_READONLY, 0, 0, NULL);
+    CloseHandle(file);

    if (map == NULL)
    {
        trace::warning(_X("Failed to map file. CreateFileMappingW(%s) failed with error %d"), path.c_str(), GetLastError());


CloseHandle above may alter GetLastError. If that happens, the last error in the log will be bogus.

jkotas · 2020-01-28T03:25:33Z

src/installer/corehost/common/pal.windows.cpp


    if (map == NULL)
    {
        trace::warning(_X("Failed to map file. CreateFileMappingW(%s) failed with error %d"), path.c_str(), GetLastError());
-        CloseHandle(file);
        return nullptr;
    }



Do you also need to close map handle before the function returns?

There is no API to close a File Mapping. The UnMapViewOfFile documentation says:
"Although an application may close the file handle used to create a file mapping object, the system holds the corresponding file open until the last view of the file is unmapped. Files for which the last view has not yet been unmapped are held open with no sharing restrictions."

CreateFileMapping documentation says:
"Mapped views of a file mapping object maintain internal references to the object, and a file mapping object does not close until all references to it are released. Therefore, to fully close a file mapping object, an application must unmap all mapped views of the file mapping object by calling UnmapViewOfFile and close the file mapping object handle by calling CloseHandle. These functions can be called in any order."

Therefore, I think map handle cannot be closed; the sharing violation was because of the FileHandle.

The documentation you have quoted says that you should close the map handle using CloseHandle.

You should close it even if it does not cause any obvious problem like sharing violation. It is good practice to avoid leaking resources.

Thanks @jkotas. I misunderstood the text to mean that the mapping handle is automatically closed after all the references are unmapped. I've fixed it now.

lpereira · 2020-02-07T21:27:41Z

src/installer/corehost/common/pal.windows.cpp

+    if (address == NULL)
    {
        trace::warning(_X("Failed to map file. MapViewOfFile(%s) failed with error %d"), path.c_str(), GetLastError());
        CloseHandle(file);
+        CloseHandle(map);
        return nullptr;
    }

+    // The file-handle (file) and mapping object handle (map) can be safely closed
+    // once the file is mapped. The OS keeps the file open if there is an open mapping into the file.
+
+    CloseHandle(file);
+    CloseHandle(map);
+
    return address;


Nitpick, but this could be written as:

if (address == nullptr) { trace::warning(...); } // ... CloseHandle(file); CloseHandle(map); return address;

(Slightly shorter code.)

lpereira · 2020-02-07T21:32:03Z

src/installer/test/TestUtils/TestProjectFixture.cs

@@ -291,6 +292,11 @@ private void ValidateRequiredDirectories(RepoDirectoriesProvider repoDirectories
                publishArgs.Add(outputDirectory);
            }

+            if (singleFile)
+            {
+                publishArgs.Add($"/p:PublishSingleFile=true");


~~Nitpick: do we need string interpolation here?~~ (Nevermind. This is just for testing, so it's OK.)

lpereira · 2020-02-07T21:36:16Z

LGTM sans a nit.

swaroop-sridhar · 2020-02-07T22:16:13Z

Thanks @lpereira ; I've made the changes you suggested.
I also made another small change to print mmap() failures as trace::error() instead of trace::warning() in the host trace.

swaroop-sridhar · 2020-02-14T07:37:53Z

@jkotas @lpereira do you have any further concerns/suggestions for this PR?
Thanks.

vitek-karas · 2020-02-14T20:24:09Z

src/installer/corehost/cli/hostmisc/pal.windows.cpp

+    // once the file is mapped. The OS keeps the file open if there is an open mapping into the file.
+
+    CloseHandle(file);
+    CloseHandle(map);


Nit: It would make more sense to swap the order - we first open the file and then create the mapping, so closing them should be done in reverse order. Thus close the mapping first and then close the file. (I know it doesn't matter as the OS will do the right thing in both cases anyway).

vitek-karas · 2020-02-14T20:24:40Z

src/installer/test/Assets/TestProjects/AppWithWait/Program.cs

@@ -0,0 +1,16 @@
+using System;


Can you please fix that in a followup PR? Or at least create an issue for it.

When running a single-file app, the AppHost mmap()s itself in order to read its contents and extract the embedded contents. The Apphost must always map its contents in order to read the headers, but doesn't always extract the contents, because previously extracted files are re-used when available. In the case where apphost doesn't extract, the file mapping wasn't immediately closed on Windows. This prevents the app from being renamed while running -- an idiom used while updating the app in-place.

ghost · 2020-02-14T21:14:37Z

Hello @swaroop-sridhar!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

swaroop-sridhar · 2020-02-14T21:15:09Z

@msftbot Squash and merge once testing succeeds.

elinor-fung · 2020-02-14T22:07:56Z

src/installer/test/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs

@@ -22,6 +22,11 @@ public static string GetAppPath(TestProjectFixture fixture)
            return Path.Combine(GetPublishPath(fixture), GetAppName(fixture));
        }

+        public static string GetPublishedSingleFilePath(TestProjectFixture fixture)
+        {
+            return GetHostPath(fixture);


Do we really need a separate function for this if all it is calling is GetHostPath?

I do think it is useful to have the wrapper call. In the single-file tests, we generate single-file apps in two ways:

Through the SDK (dotnet publish /p:PublishSingleFile=true)

Through invoking HostModel functions directly.

In the former case GetHostPath() will be the pre-bundled app, whereas in the later case, GetHostPath() will just be the host. So, I think it is easier to refer to them by different names.
Helps documenting the intent, and in searching for the use with different semantics.

Ahh, okay. I didn't realize the different use cases and just assumed the paths in BundleHelper were for the pre-bundled app.

dotnet/runtime#1260 A single-file app cannot be renamed while running -- an idiom used while updating the app in-place. When running a single-file app, the AppHost reads itself in order to extract the embedded contents. The Apphost must always read its contents in order to read the headers, but doesn't always extract the contents, because previously extracted files are re-used when available. In the case where apphost doesn't extract, currently, the file stream isn't immediately closed on Windows. This prevents the app from being renamed while running. This change fixes the problem by closing the open stream in all cases. Very Low dotnet/runtime#2272

jkotas reviewed Jan 28, 2020

View reviewed changes

swaroop-sridhar force-pushed the unmap branch from 47743ac to dd4e9d3 Compare January 28, 2020 23:58

maryamariyan added the area-Single-File label Jan 30, 2020

swaroop-sridhar requested a review from lpereira February 7, 2020 21:02

swaroop-sridhar force-pushed the unmap branch from c25ae8e to 8efa026 Compare February 7, 2020 21:04

lpereira reviewed Feb 7, 2020

View reviewed changes

swaroop-sridhar force-pushed the unmap branch from 8efa026 to 071e047 Compare February 7, 2020 22:15

swaroop-sridhar force-pushed the unmap branch 3 times, most recently from 1932524 to 75ef751 Compare February 14, 2020 03:38

jkotas requested review from vitek-karas and elinor-fung February 14, 2020 16:21

vitek-karas approved these changes Feb 14, 2020

View reviewed changes

swaroop-sridhar force-pushed the unmap branch from 75ef751 to 4f45b06 Compare February 14, 2020 20:51

swaroop-sridhar force-pushed the unmap branch from 4f45b06 to 3fa15c9 Compare February 14, 2020 21:02

swaroop-sridhar added the auto-merge label Feb 14, 2020

elinor-fung approved these changes Feb 14, 2020

View reviewed changes

ghost merged commit b95e523 into dotnet:master Feb 14, 2020

swaroop-sridhar mentioned this pull request Feb 28, 2020

[release/3.1] Single-File: Close AppHost stream when reusing extraction dotnet/core-setup#9010

Merged

swaroop-sridhar mentioned this pull request Mar 9, 2020

Add License Header to Installer Test Assets #33408

Merged

jkotas mentioned this pull request May 9, 2020

Make Process.Start have a option to change handle inheritance #13943

Open

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SingleFile bundles: Ensure extraction mappings are closed on Windows. #2272

SingleFile bundles: Ensure extraction mappings are closed on Windows. #2272

swaroop-sridhar commented Jan 28, 2020

jkotas Jan 28, 2020

swaroop-sridhar Jan 28, 2020

vitek-karas Feb 14, 2020

swaroop-sridhar Feb 14, 2020 •

edited

Loading

jkotas Jan 28, 2020

jkotas Jan 28, 2020

jkotas Jan 28, 2020

swaroop-sridhar Jan 28, 2020

jkotas Jan 28, 2020 •

edited

Loading

swaroop-sridhar Jan 29, 2020

lpereira Feb 7, 2020

lpereira Feb 7, 2020 •

edited

Loading

lpereira commented Feb 7, 2020

swaroop-sridhar commented Feb 7, 2020

swaroop-sridhar commented Feb 14, 2020

vitek-karas Feb 14, 2020

vitek-karas Feb 14, 2020

ghost commented Feb 14, 2020

swaroop-sridhar commented Feb 14, 2020

elinor-fung Feb 14, 2020

swaroop-sridhar Feb 14, 2020

elinor-fung Feb 14, 2020

		@@ -109,7 +109,6 @@ void* pal::map_file_readonly(const pal::string_t& path, size_t &length)
		if (map == NULL)

SingleFile bundles: Ensure extraction mappings are closed on Windows. #2272

SingleFile bundles: Ensure extraction mappings are closed on Windows. #2272

Conversation

swaroop-sridhar commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

lpereira commented Feb 7, 2020

swaroop-sridhar commented Feb 7, 2020

swaroop-sridhar commented Feb 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 14, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

swaroop-sridhar commented Feb 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar Feb 14, 2020 •

edited

Loading

jkotas Jan 28, 2020 •

edited

Loading

lpereira Feb 7, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.