Avoid all compiler optimization on embedded apphost hash #110554

omajid · 2024-12-09T22:58:13Z

We assume that there is a single copy of the apphost hash in the apphost binary. And that it hasn't been modified by the compiler. However, the compiler can optimize the hash multiple ways, including re-ordering elements of the hash or duplicating the contents of the hash. This can currently happen under certain compiler versions and optimization flags.

Try and avoid that by marking the hash as a volatile string and implementing comparisons/copying/initialization that respects that.

Fixes: #109611

We assume that there is a single copy of the apphost hash in the apphost binary. And that it hasn't been modified by the compiler. However, the compiler can optimize the hash multiple ways, including re-ordering elements of the hash or duplicating the contents of the hash. This can currently happen under certain compiler versions and optimization flags. Try and avoid that by marking the hash as a volatile string and implementing comparisons/copying/initialization that respects that. Fixes: dotnet#109611

omajid · 2024-12-12T15:15:15Z

src/native/corehost/corehost.cpp


    static const char hi_part[] = EMBED_HASH_HI_PART_UTF8;
    static const char lo_part[] = EMBED_HASH_LO_PART_UTF8;

-    if (!pal::clr_palstring(embed, app_dll))


Implementing pal::clr_palstring per-platform to handle volatile correctly looked much more involved that creating a non-volatile copy instead.

Strangely enough, even the non-volatile copy still needed the no-opt memory compare to stop creating duplicate instances of the embedded hash. I wonder if the compiler ignored volatile data when it started optimizing the non-volatile copy?

My guess is that the compiler can see all operations on the value and that the value does not escape, and thus volatile can be safely ignored.

It may help to move the value to the global scope and mark it as extern or extern "C". It should prevent the compiler from making assumptions the value.

tmds · 2024-12-12T17:36:22Z

src/native/corehost/corehost.cpp

@@ -48,18 +66,21 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll)
    // Contains the EMBED_HASH_FULL_UTF8 value at compile time or the managed DLL name replaced by "dotnet build".
    // Must not be 'const' because std::string(&embed[0]) below would bind to a const string ctor plus length
    // where length is determined at compile time (=64) instead of the actual length of the string at runtime.
-    static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8;     // series of NULs followed by embed hash string
+    volatile static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8;     // series of NULs followed by embed hash string


I'm curious what happens if you change:

static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string

to:

static const char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; char embed[EMBED_MAX]; to_non_volatile(const_embed, embed, EMBED_MAX);

And keep the remainder of the file as it were (except for adding to_non_volatile).

The comment above says we can't use const.

I made this change as the full diff:

diff --git a/src/native/corehost/corehost.cpp b/src/native/corehost/corehost.cpp index 6de7acfbd08..d0c90f4bfcf 100644 --- a/src/native/corehost/corehost.cpp +++ b/src/native/corehost/corehost.cpp @@ -40,6 +40,24 @@ #define EMBED_HASH_LO_PART_UTF8 "74e592c2fa383d4a3960714caef0c4f2" #define EMBED_HASH_FULL_UTF8 (EMBED_HASH_HI_PART_UTF8 EMBED_HASH_LO_PART_UTF8) // NUL terminated +void to_non_volatile(volatile const char* cstr, char* output, size_t length) +{ + for (size_t i = 0; i < length; i++) + { + output[i] = cstr[i]; + } +} + +bool compare_memory_nooptimization(volatile const char* a, volatile const char* b, size_t length) +{ + for (size_t i = 0; i < length; i++) + { + if (*a++ != *b++) + return false; + } + return true; +} + bool is_exe_enabled_for_execution(pal::string_t* app_dll) { constexpr int EMBED_SZ = sizeof(EMBED_HASH_FULL_UTF8) / sizeof(EMBED_HASH_FULL_UTF8[0]); @@ -48,11 +66,14 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll) // Contains the EMBED_HASH_FULL_UTF8 value at compile time or the managed DLL name replaced by "dotnet build". // Must not be 'const' because std::string(&embed[0]) below would bind to a const string ctor plus length // where length is determined at compile time (=64) instead of the actual length of the string at runtime. - static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string + volatile static char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string static const char hi_part[] = EMBED_HASH_HI_PART_UTF8; static const char lo_part[] = EMBED_HASH_LO_PART_UTF8; + char embed[EMBED_MAX]; + to_non_volatile(const_embed, embed, EMBED_MAX); + if (!pal::clr_palstring(embed, app_dll)) { trace::error(_X("The managed DLL bound to this executable could not be retrieved from the executable image."));

The compiler embeds the string twice:

++ grep --byte-offset --only-matching --text c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca ./artifacts/obj/coreclr/linux.x64.Release/Corehost.Static/CMakeFiles/singlefilehost.dir/home/omajid/runtime/src/native/corehost/corehost.cpp.o 320:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca 4816:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca

Using const static char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; produces the exact same result.

The comment above says we can't use const.

Like this, it is also working around a compiler optimization. I thought we may tackle both together.

Using const static char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; produces the exact same result.

That is: you also tried without the volatile?

I'm surprised it found the need to duplicate const data which shouldn't be optimized for a specific usage.

Thanks for giving it try!

No idea what is triggering the compiler...

One more suggestion/attempt:

Is the original suggestion https://github.com/dotnet/runtime/pull/110554/files#r1882606833 enough if you use volatile static char const_embed (like the PR is doing already) rather than static const char const_embed?

You mean like this?

diff --git a/src/native/corehost/corehost.cpp b/src/native/corehost/corehost.cpp index 6de7acfbd08..f77fbe63347 100644 --- a/src/native/corehost/corehost.cpp +++ b/src/native/corehost/corehost.cpp @@ -40,6 +40,14 @@ #define EMBED_HASH_LO_PART_UTF8 "74e592c2fa383d4a3960714caef0c4f2" #define EMBED_HASH_FULL_UTF8 (EMBED_HASH_HI_PART_UTF8 EMBED_HASH_LO_PART_UTF8) // NUL terminated +void to_non_volatile(volatile const char* cstr, char* output, size_t length) +{ + for (size_t i = 0; i < length; i++) + { + output[i] = cstr[i]; + } +} + bool is_exe_enabled_for_execution(pal::string_t* app_dll) { constexpr int EMBED_SZ = sizeof(EMBED_HASH_FULL_UTF8) / sizeof(EMBED_HASH_FULL_UTF8[0]); @@ -48,11 +56,14 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll) // Contains the EMBED_HASH_FULL_UTF8 value at compile time or the managed DLL name replaced by "dotnet build". // Must not be 'const' because std::string(&embed[0]) below would bind to a const string ctor plus length // where length is determined at compile time (=64) instead of the actual length of the string at runtime. - static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string + volatile static const char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string static const char hi_part[] = EMBED_HASH_HI_PART_UTF8; static const char lo_part[] = EMBED_HASH_LO_PART_UTF8; + char embed[EMBED_MAX]; + to_non_volatile(const_embed, embed, EMBED_MAX); + if (!pal::clr_palstring(embed, app_dll)) { trace::error(_X("The managed DLL bound to this executable could not be retrieved from the executable image."));

This leads to:

$ grep --byte-offset --only-matching --text c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca ./artifacts/obj/coreclr/linux.x64.Release/Corehost.Static/CMakeFiles/singlefilehost.dir/home/omajid/runtime/src/native/corehost/corehost.cpp.o 256:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca 4752:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca

const vs non-const doesn't make a difference, as far as I can see:

diff --git a/src/native/corehost/corehost.cpp b/src/native/corehost/corehost.cpp index 6de7acfbd08..7c6c1244815 100644 --- a/src/native/corehost/corehost.cpp +++ b/src/native/corehost/corehost.cpp @@ -40,6 +40,14 @@ #define EMBED_HASH_LO_PART_UTF8 "74e592c2fa383d4a3960714caef0c4f2" #define EMBED_HASH_FULL_UTF8 (EMBED_HASH_HI_PART_UTF8 EMBED_HASH_LO_PART_UTF8) // NUL terminated +void to_non_volatile(volatile const char* cstr, char* output, size_t length) +{ + for (size_t i = 0; i < length; i++) + { + output[i] = cstr[i]; + } +} + bool is_exe_enabled_for_execution(pal::string_t* app_dll) { constexpr int EMBED_SZ = sizeof(EMBED_HASH_FULL_UTF8) / sizeof(EMBED_HASH_FULL_UTF8[0]); @@ -48,11 +56,14 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll) // Contains the EMBED_HASH_FULL_UTF8 value at compile time or the managed DLL name replaced by "dotnet build". // Must not be 'const' because std::string(&embed[0]) below would bind to a const string ctor plus length // where length is determined at compile time (=64) instead of the actual length of the string at runtime. - static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string + volatile static char const_embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string static const char hi_part[] = EMBED_HASH_HI_PART_UTF8; static const char lo_part[] = EMBED_HASH_LO_PART_UTF8; + char embed[EMBED_MAX]; + to_non_volatile(const_embed, embed, EMBED_MAX); + if (!pal::clr_palstring(embed, app_dll)) { trace::error(_X("The managed DLL bound to this executable could not be retrieved from the executable image."));

$ grep --byte-offset --only-matching --text c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca ./artifacts/obj/coreclr/linux.x64.Release/Corehost.Static/CMakeFiles/singlefilehost.dir/home/omajid/runtime/src/native/corehost/corehost.cpp.o 256:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca 4752:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca

My conclusion is that I neither understand why all of these don't work, and why what is in the PR does work. 😅

I didn't realize the duplication comes from the compares against the parts and not from the complete embed...

I wonder if introducing overlap in the parts wouldn't eliminate the issue too.

diff --git a/src/native/corehost/corehost.cpp b/src/native/corehost/corehost.cpp index 6de7acfbd08..e3d0ca72891 100644 --- a/src/native/corehost/corehost.cpp +++ b/src/native/corehost/corehost.cpp @@ -36,9 +36,10 @@ * o https://en.wikipedia.org/wiki/Comparison_of_file_systems * has more details on maximum file name sizes. */ -#define EMBED_HASH_HI_PART_UTF8 "c3ab8ff13720e8ad9047dd39466b3c89" // SHA-256 of "foobar" in UTF-8 -#define EMBED_HASH_LO_PART_UTF8 "74e592c2fa383d4a3960714caef0c4f2" -#define EMBED_HASH_FULL_UTF8 (EMBED_HASH_HI_PART_UTF8 EMBED_HASH_LO_PART_UTF8) // NUL terminated +#define EMBED_HASH_PART_1_UTF8 "c3ab8ff13720e8ad9047dd39" // SHA-256 of "foobar" in UTF-8 +#define EMBED_HASH_PART_2_UTF8 "466b3c8974e592c2fa383d4a" +#define EMBED_HASH_PART_3_UTF8 "3960714caef0c4f2" +#define EMBED_HASH_FULL_UTF8 (EMBED_HASH_PART_1_UTF8 EMBED_HASH_PART_2_UTF8 EMBED_HASH_PART_3_UTF8) // NUL terminated bool is_exe_enabled_for_execution(pal::string_t* app_dll) { @@ -50,8 +51,8 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll) // where length is determined at compile time (=64) instead of the actual length of the string at runtime. static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8; // series of NULs followed by embed hash string - static const char hi_part[] = EMBED_HASH_HI_PART_UTF8; - static const char lo_part[] = EMBED_HASH_LO_PART_UTF8; + static const char hi_part[] = (EMBED_HASH_PART_1_UTF8 EMBED_HASH_PART_2_UTF8); + static const char lo_part[] = (EMBED_HASH_PART_2_UTF8 EMBED_HASH_PART_3_UTF8); if (!pal::clr_palstring(embed, app_dll)) { @@ -73,9 +74,9 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll) // So use two parts of the string that will be unaffected by the edit. size_t hi_len = (sizeof(hi_part) / sizeof(hi_part[0])) - 1; size_t lo_len = (sizeof(lo_part) / sizeof(lo_part[0])) - 1; - if (binding.size() >= (hi_len + lo_len) + if (binding.size() >= (EMBED_SZ - 1) && binding.compare(0, hi_len, &hi_part[0]) == 0 - && binding.compare(hi_len, lo_len, &lo_part[0]) == 0) + && binding.compare(EMBED_SZ - 1 - lo_len, lo_len, &lo_part[0]) == 0) { trace::error(_X("This executable is not bound to a managed DLL to execute. The binding value is: '%s'"), app_dll->c_str()); return false;

omajid · 2024-12-12T19:59:38Z

Rewriting volatile-based implementations instead of std::string also seems to work:

diff --git a/src/native/corehost/corehost.cpp b/src/native/corehost/corehost.cpp
index 6de7acfbd08..d5e5fc3fb61 100644
--- a/src/native/corehost/corehost.cpp
+++ b/src/native/corehost/corehost.cpp
@@ -40,6 +40,33 @@
 #define EMBED_HASH_LO_PART_UTF8 "74e592c2fa383d4a3960714caef0c4f2"
 #define EMBED_HASH_FULL_UTF8    (EMBED_HASH_HI_PART_UTF8 EMBED_HASH_LO_PART_UTF8) // NUL terminated
 
+void to_non_volatile(volatile const char* cstr, char* output, size_t length)
+{
+    for (size_t i = 0; i < length; i++)
+    {
+        output[i] = cstr[i];
+    }
+}
+
+bool compare_memory_nooptimization(volatile const char* a, volatile const char* b, size_t length)
+{
+    for (size_t i = 0; i < length; i++)
+    {
+        if (*a++ != *b++)
+            return false;
+    }
+    return true;
+}
+
+size_t strlen_nooptimization(volatile const char* a)
+{
+    for (size_t i = 0; ; i++)
+    {
+       if (a[i] == 0)
+               return i;
+    }
+}
+
 bool is_exe_enabled_for_execution(pal::string_t* app_dll)
 {
     constexpr int EMBED_SZ = sizeof(EMBED_HASH_FULL_UTF8) / sizeof(EMBED_HASH_FULL_UTF8[0]);
@@ -48,21 +75,24 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll)
     // Contains the EMBED_HASH_FULL_UTF8 value at compile time or the managed DLL name replaced by "dotnet build".
     // Must not be 'const' because std::string(&embed[0]) below would bind to a const string ctor plus length
     // where length is determined at compile time (=64) instead of the actual length of the string at runtime.
-    static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8;     // series of NULs followed by embed hash string
+    volatile static char embed[EMBED_MAX] = EMBED_HASH_FULL_UTF8;     // series of NULs followed by embed hash string
 
     static const char hi_part[] = EMBED_HASH_HI_PART_UTF8;
     static const char lo_part[] = EMBED_HASH_LO_PART_UTF8;
 
-    if (!pal::clr_palstring(embed, app_dll))
+    char embed_non_volatile[EMBED_MAX];
+    to_non_volatile(embed, embed_non_volatile, EMBED_MAX);
+
+    if (!pal::clr_palstring(embed_non_volatile, app_dll))
     {
         trace::error(_X("The managed DLL bound to this executable could not be retrieved from the executable image."));
         return false;
     }
 
-    std::string binding(&embed[0]);
+    size_t length = strlen_nooptimization(embed);
 
     // Check if the path exceeds the max allowed size
-    if (binding.size() > EMBED_MAX - 1) // -1 for null terminator
+    if (length > EMBED_MAX - 1) // -1 for null terminator
     {
         trace::error(_X("The managed DLL bound to this executable is longer than the max allowed length (%d)"), EMBED_MAX - 1);
         return false;
@@ -73,9 +103,9 @@ bool is_exe_enabled_for_execution(pal::string_t* app_dll)
     // So use two parts of the string that will be unaffected by the edit.
     size_t hi_len = (sizeof(hi_part) / sizeof(hi_part[0])) - 1;
     size_t lo_len = (sizeof(lo_part) / sizeof(lo_part[0])) - 1;
-    if (binding.size() >= (hi_len + lo_len)
-        && binding.compare(0, hi_len, &hi_part[0]) == 0
-        && binding.compare(hi_len, lo_len, &lo_part[0]) == 0)
+    if (length >= (hi_len + lo_len)
+        && compare_memory_nooptimization(embed, hi_part, hi_len)
+        && compare_memory_nooptimization(embed + hi_len, lo_part, lo_len))
     {
         trace::error(_X("This executable is not bound to a managed DLL to execute. The binding value is: '%s'"), app_dll->c_str());
         return false;

$ grep --byte-offset --only-matching --text c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca ./artifacts/obj/coreclr/linux.x64.Release/Corehost.Static/CMakeFiles/singlefilehost.dir/home/omajid/runtime/src/native/corehost/corehost.cpp.o
5696:c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714ca

dotnet-issue-labeler bot added the area-Host label Dec 9, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 9, 2024

omajid force-pushed the volatile-apphost-ash branch from 1ebf110 to 81e3213 Compare December 10, 2024 00:16

omajid force-pushed the volatile-apphost-ash branch 2 times, most recently from 74ea3b1 to aa103bb Compare December 10, 2024 16:57

This was referenced Dec 10, 2024

Error reported in diagnostic logs. Please examine the log for more details dotnet/dnceng#1928

Open

Test failures in JSImportGenerator.Unit.Tests.Compiles.ValidateGeneratedSourceOutput #110575

Closed

omajid force-pushed the volatile-apphost-ash branch from aa103bb to 265d78f Compare December 10, 2024 22:11

build-analysis bot mentioned this pull request Dec 11, 2024

System.Formats.Nrbf.Tests timeouts #110285

Closed

omajid force-pushed the volatile-apphost-ash branch from 265d78f to 8d4a245 Compare December 11, 2024 22:02

omajid force-pushed the volatile-apphost-ash branch from 8d4a245 to 4e33337 Compare December 11, 2024 22:29

build-analysis bot mentioned this pull request Dec 12, 2024

[wasm] Out of memory exception in System.Xml.XmlWriterApiTests.TCFullEndElement tests #110090

Open

omajid marked this pull request as ready for review December 12, 2024 15:14

omajid commented Dec 12, 2024

View reviewed changes

tmds reviewed Dec 12, 2024

View reviewed changes

AaronRobinsonMSFT requested a review from elinor-fung December 12, 2024 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid all compiler optimization on embedded apphost hash #110554

Avoid all compiler optimization on embedded apphost hash #110554

omajid commented Dec 9, 2024

omajid Dec 12, 2024 •

edited

Loading

jkotas Dec 12, 2024 •

edited

Loading

tmds Dec 12, 2024

omajid Dec 12, 2024

omajid Dec 12, 2024

omajid Dec 12, 2024

tmds Dec 12, 2024

tmds Dec 12, 2024 •

edited

Loading

omajid Dec 12, 2024

omajid Dec 12, 2024

tmds Dec 12, 2024 •

edited

Loading

tmds Dec 12, 2024

omajid commented Dec 12, 2024

Avoid all compiler optimization on embedded apphost hash #110554

Are you sure you want to change the base?

Avoid all compiler optimization on embedded apphost hash #110554

Conversation

omajid commented Dec 9, 2024

omajid Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

jkotas Dec 12, 2024 • 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

tmds Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omajid commented Dec 12, 2024

omajid Dec 12, 2024 •

edited

Loading

jkotas Dec 12, 2024 •

edited

Loading

tmds Dec 12, 2024 •

edited

Loading

tmds Dec 12, 2024 •

edited

Loading