Enable singing of single-file bundles #6885

swaroop-sridhar · 2019-06-21T00:08:43Z

This change removes the requirement that the bundle-signature
be found at the end of the file, so that tools like signtool
can append their custom information at the end of the file.

The layout of .net core bundles was

AppHost Code
Bundled Files
Manifest header
Manifest entries
Bundle footer

The bundle-footer is now removed, its functionality is served by a
special global in apphost (Bundle-Placeholder) similar to the way
APP-DLL name is encoded in the AppHost.

The normal unbundled apphost has a default value for the bundle-placeholder.
If the apphost is a single-file bundle, the bundler tool (in HostModel) updates
the value of the variable on disk to a different value, and encodes the location
of the manifest header in it.

This way, there is no requirement that the bundle meta-data must remain at
the end of the file.

This change also simplifies the test for whether an apphost is a bundle:
we only check a value in the loaded binary, rather than performing file reads.

AustinWise · 2019-06-21T17:36:41Z

src/managed/Microsoft.NET.HostModel/AppHost/HostWriter.cs


- // Copy apphost to destination path so it inherits the same attributes/permissions.
- File.Copy(appHostSourceFilePath, appHostDestinationFilePath, overwrite: true);
+ bundleHeaderOffset = accessor.ReadInt64(position - sizeof(Int64));


Perhaps this method should return false if bundleHeaderOffset is zero, like marker_t::is_bundle() does.

@AustinWise, change is in this commit: 75f1926

Thanks, that should make for a more clear error message.

lpereira · 2019-06-21T18:20:46Z

src/corehost/cli/apphost/bundle/bundle_runner.cpp

-void bundle_runner_t::process_manifest_header(int64_t header_offset)
-{
- seek(m_bundle_stream, header_offset, SEEK_SET);
+ seek(m_bundle_stream, marker_t::header_offset(), SEEK_SET);

 manifest_header_t* header = manifest_header_t::read(m_bundle_stream);


Since header is not stored anywhere, and it's allocated in manifest_header_t, the memory leaks. It might be a better idea to allocate on the stack here and have manifest_header_t::read() take a point to it as a parameter.

@lpereira please see this commit
bd597da

vitek-karas

LGTM

vitek-karas · 2019-06-24T09:26:02Z

src/corehost/cli/apphost/bundle/bundle_runner.cpp

- }
- process_manifest_header(manifest_header_offset);
+ // Read the bundle header
+ seek(m_bundle_stream, marker_t::header_offset(), SEEK_SET);


If the file is corrupt this might fail (the header offset might point beyond the end of the file) - I know that we'll catch it later on by validating the header itself, but I think for code cleanness it would be better to check the failure here as well.

It's often a good idea to replace seek()+read() with pread() if supported by the system. This way, if reading with an invalid offset, the pread() call will fail. This is the sort of thing that the PAL could do.

@vitek-karas the seek() function above already checks for failure, and throws the bundleIOerror exception. This is bundle_runner_t::seek which does fseek and the error check.

@lpereira I agree that the PAL can be improved by adding pread().
But I think this code will soon change to use memory-mapping of the whole bundle file instead of seek-read-write, etc. So, I left it as-is for now.

vitek-karas · 2019-06-24T09:56:39Z

src/corehost/cli/apphost/bundle/header.h

+#pragma pack(pop)
+ pal::string_t m_bundle_id;
+
+ static const uint32_t m_current_major_version = 0;


Since we're about to ship this - should we maybe use version 1.0? - it's not an experimental feature per se.

If it's static, should it be prefixed with m_?

No private statics should be s_, but this field can just be a const; so I'll remove the annotations.

vitek-karas · 2019-06-24T09:59:02Z

src/corehost/cli/apphost/bundle/header.cpp

+
+ // bundle_id is a component of the extraction path
+ size_t bundle_id_length = 
+ bundle_runner_t::get_path_length(header->m_data.bundle_id_length_byte_1, stream);


I find this a bit weird - naming wise - bundle_id itself is not a path... so using methods like get_path_length doesn't feel right. Maybe we should rename the method to simply get_string_length or something similar. Especially since we have read_string already.

bundle_id is actually a part of a path (that is, it should be path-compatible, and conform to length limitation). The get_path_length method only reads two bytes in a length encoding (because of MAX_PATH) restriction. So, that's why I had called it get_path_length instead of get_string_length

I can rewrite the function more generically, call it read_string_length and do the MAX_PATH check elsewhere -- but it doesn't serve much purpose other than the symmetry with read_string() function.

OK - it was just a nit anyway... :-)

lpereira · 2019-06-24T17:39:04Z

src/corehost/cli/apphost/bundle/bundle_runner.h

@@ -50,10 +52,9 @@ namespace bundle
 void extract_file(file_entry_t* entry);

 FILE* m_bundle_stream;
+ header_t* m_header;


What is destroying m_header? The memory leak is still here. A few options, in order from the most preferred to the least preferred:

Declare this inline as header_t m_header. No need to allocate a new struct from the heap. Goes away as soon as bundle_runner_t goes away.

Make this a std::unique_ptr<header_t> instead.

Have a delete m_header in bundle_runner_t::~bundle_runner_t().

I added a destructor to the bundle_runner_t.
I did not inline the manifest, because when files are directly processed from the bundle, the manifest will survive beyond the bundle_runner object.
I also wanted to keep the interface of header_t similar to manifest_t and the managed code in HostModel. Thanks.

This change removes the requirement that the bundle-signature be found at the end of the file, so that tools like signtool can append their custom information at the end of the file. The layout of .net core bundles was AppHost Code Bundled Files Manifest header Manifest entries Bundle footer <EOF> The bundle-footer is now removed, its functionality is served by a special global in apphost (Bundle-Placeholder) similar to the way APP-DLL name is encoded in the AppHost. The normal unbundled apphost has a default value for the bundle-placeholder. If the apphost is a single-file bundle, the bundler tool (in HostModel) updates the value of the variable on disk to a different value, and encodes the location of the manifest header in it. This way, there is no requirement that the bundle meta-data must remain at the end of the file. This change also simplifies the test for whether an apphost is a bundle: we only check a value in the loaded binary, rather than performing file reads.

Changes a storage class so that constants are held without warning.

Store bundle_header* in bundle_runner so that it is destructed after bundle processing (and eliminates redundant storage of bundle_header contents in bundle_runner).

To fix a build error on linux.

Leave an error code unused (rather than rewriting) to fix a failure in the lab jobs Also fix a spacing

The static placeholder array was optimized out in OSX release builds. This caused tests in release builds to fail with PlaceholderNotFound exception, whereas the debug builds passed.

swaroop-sridhar · 2019-06-26T07:21:00Z

The win-x64 debug failure says System.IO.IOException : There is not enough space on the disk : 'D:\a\1\s\bin\tests\win-x64.Debug\8\StandaloneApp\publish\StandaloneApp.exe'
The error happens when the fixture is cloned for use, likely to be a testing issue.

Disable this test since it is causing other tests to fail because of System.IO.IOException : There is not enough space on the disk This looks like an infrastructure issue, which I will follow up.

dagood · 2019-06-26T18:45:08Z

I hit the There is not enough space on the disk errors in my work to migrate to Arcade, too, and moving off the tiny hosted agents onto the Helix queues worked for me: cbe76ff. IMO feel free to do so here.

swaroop-sridhar · 2019-06-26T18:49:44Z

Thanks @dagood ; I'll try that (in a separate change).

swaroop-sridhar · 2019-06-26T18:51:44Z

The Build_Windows_x64 debug leg passed with the test disabled.
A few more legs are waiting for machines to run on, but have previously passed.
https://dev.azure.com/dnceng/public/_build/results?buildId=239197

So, I'll merge the change, so that the fix is available in the upcoming preview.

The above test was disabled in PR dotnet#6885 because the lab runs showed failures because of "not enough disk space" This PR tries to enable the test, and move the job to not use the hosted pool, as suggested [here](dotnet#6885 (comment)).

The above test was disabled in PR dotnet#6885 because the lab runs showed failures because of "not enough disk space" This PR now enables the test, since the job no longer uses the hosted pool.

The above test was disabled in PR dotnet#6885 because the lab runs showed failures because of "not enough disk space" This PR now enables the test, since the test job no longer uses the hosted pool.

The above test was disabled in PR #6885 because the lab runs showed failures because of "not enough disk space" This PR now enables the test, since the test job no longer uses the hosted pool.

nxtn · 2020-11-18T12:16:42Z

src/corehost/cli/apphost/bundle/marker.cpp

+int64_t marker_t::header_offset()
+{
+ // Contains the bundle_placeholder default value at compile time.
+ // If this is a single-file bundle, the last 8 bytes are replaced 


I believe it is the first 8 bytes that are replaced?

Good catch - thanks a lot!
dotnet/runtime#44874

* Enable singing of bundles This change removes the requirement that the bundle-signature be found at the end of the file, so that tools like signtool can append their custom information at the end of the file. The layout of .net core bundles was AppHost Code Bundled Files Manifest header Manifest entries Bundle footer <EOF> The bundle-footer is now removed, its functionality is served by a special global in apphost (Bundle-Placeholder) similar to the way APP-DLL name is encoded in the AppHost. The normal unbundled apphost has a default value for the bundle-placeholder. If the apphost is a single-file bundle, the bundler tool (in HostModel) updates the value of the variable on disk to a different value, and encodes the location of the manifest header in it. This way, there is no requirement that the bundle meta-data must remain at the end of the file. This change also simplifies the test for whether an apphost is a bundle: we only check a value in the loaded binary, rather than performing file reads. An additional change in this commit is that the Bundle version is changed from 0.1 to 1.0 This change adds a test case: TestWithAdditionalContentAfterBundleMetadata However, this caused a different test to fail in the lab because the Disable this test since it is causing other tests to fail because the machines ran out of disk space copying the test assets. So, this test is temporarily disabled, until the infrastructure issue is resolved (likely by not using the hosted-pool for testing). Commit migrated from dotnet/core-setup@19b99b3

…/core-setup#6968) The above test was disabled in PR dotnet/core-setup#6885 because the lab runs showed failures because of "not enough disk space" This PR now enables the test, since the test job no longer uses the hosted pool. Commit migrated from dotnet/core-setup@b44e5a2

AustinWise reviewed Jun 21, 2019

View reviewed changes

lpereira suggested changes Jun 21, 2019

View reviewed changes

swaroop-sridhar changed the title ~~WIP: Enable singing of bundles~~ Enable singing of bundles Jun 21, 2019

vitek-karas approved these changes Jun 24, 2019

View reviewed changes

lpereira suggested changes Jun 24, 2019

View reviewed changes

swaroop-sridhar changed the title ~~Enable singing of bundles~~ Enable singing of single-file bundles Jun 24, 2019

swaroop-sridhar mentioned this pull request Jun 24, 2019

.netcore single file executable codesign fail on macOS dotnet/core#2811

Closed

swaroop-sridhar added 10 commits June 25, 2019 13:05

Address feedback by AustinWise

87bad6a

Fix a build error in test runs

1946ce9

Changes a storage class so that constants are held without warning.

Address comments by @lpereira

c1f32d7

Store bundle_header* in bundle_runner so that it is destructed after bundle processing (and eliminates redundant storage of bundle_header contents in bundle_runner).

Add a test case

954f3f8

Some additional cleanup

0cf4bc4

Fix a constant naming

b4337e6

Rewrite initialization of placeholder

416a70b

To fix a build error on linux.

Change version to 1.0

02dacec

Add destructor for bundle_runner

7c28397

swaroop-sridhar force-pushed the header branch from e30fb28 to 7c28397 Compare June 25, 2019 20:05

Update Error codes

9a72a8e

Leave an error code unused (rather than rewriting) to fix a failure in the lab jobs Also fix a spacing

swaroop-sridhar force-pushed the header branch from 608a17b to fe0b07c Compare June 26, 2019 00:17

Make the bundle placeholder volatile

ad198de

The static placeholder array was optimized out in OSX release builds. This caused tests in release builds to fail with PlaceholderNotFound exception, whereas the debug builds passed.

swaroop-sridhar force-pushed the header branch from fe0b07c to ad198de Compare June 26, 2019 01:45

swaroop-sridhar mentioned this pull request Jun 26, 2019

[Test Only] WIP Fix for a test failure in #6885 #6933

Closed

Temporarily disable TestWithAdditionalContentAfterBundleMetadata

92d1b2b

Disable this test since it is causing other tests to fail because of System.IO.IOException : There is not enough space on the disk This looks like an infrastructure issue, which I will follow up.

swaroop-sridhar merged commit 19b99b3 into dotnet:master Jun 26, 2019

swaroop-sridhar mentioned this pull request Jun 27, 2019

HostModel:Enable TestWithAdditionalContentAfterBundleMetadata #6968

Merged

nxtn reviewed Nov 18, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable singing of single-file bundles #6885

Enable singing of single-file bundles #6885

swaroop-sridhar commented Jun 21, 2019

AustinWise Jun 21, 2019 •

edited

Loading

swaroop-sridhar Jun 21, 2019

AustinWise Jun 22, 2019

lpereira Jun 21, 2019

swaroop-sridhar Jun 21, 2019

vitek-karas left a comment

vitek-karas Jun 24, 2019

lpereira Jun 24, 2019 •

edited

Loading

swaroop-sridhar Jun 25, 2019 •

edited

Loading

swaroop-sridhar Jun 25, 2019

vitek-karas Jun 24, 2019

lpereira Jun 24, 2019

swaroop-sridhar Jun 24, 2019

vitek-karas Jun 24, 2019

swaroop-sridhar Jun 24, 2019

vitek-karas Jun 24, 2019

lpereira Jun 24, 2019 •

edited

Loading

swaroop-sridhar Jun 25, 2019

swaroop-sridhar commented Jun 26, 2019

dagood commented Jun 26, 2019

swaroop-sridhar commented Jun 26, 2019

swaroop-sridhar commented Jun 26, 2019

nxtn Nov 18, 2020

vitek-karas Nov 18, 2020

Enable singing of single-file bundles #6885

Enable singing of single-file bundles #6885

Conversation

swaroop-sridhar commented Jun 21, 2019

AustinWise Jun 21, 2019 • 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

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

swaroop-sridhar Jun 25, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpereira Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar commented Jun 26, 2019

dagood commented Jun 26, 2019

swaroop-sridhar commented Jun 26, 2019

swaroop-sridhar commented Jun 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AustinWise Jun 21, 2019 •

edited

Loading

lpereira Jun 24, 2019 •

edited

Loading

swaroop-sridhar Jun 25, 2019 •

edited

Loading

lpereira Jun 24, 2019 •

edited

Loading