From 2b0937523254bbf0fdfcb2726492c4396eeaaa14 Mon Sep 17 00:00:00 2001
From: Jan Vorlicek <janvorli@microsoft.com>
Date: Thu, 2 Apr 2020 12:37:58 +0200
Subject: [PATCH] Enhance PAL initialization order (#34308)

* Enhance PAL initialization order

The CGroups initialization has _ASSERTE that is not asserting because
debugging support was being initialized after the CGroups initialization.

This change moves the debugging support initialization to the earliest
possible point. It also modifies it so that it doesn't depend on the
PAL env functions and uses getenv instead. And finally, initialization
of the critical section for debug printf is moved to the very end
of DBG_init_channels just as a little cleanup.
---
 src/coreclr/src/pal/src/init/pal.cpp    | 35 ++++++++++++++++-----------------
 src/coreclr/src/pal/src/misc/dbgmsg.cpp | 30 +++++++++++-----------------
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/src/coreclr/src/pal/src/init/pal.cpp b/src/coreclr/src/pal/src/init/pal.cpp
index 6270f1c..b25924e 100644
--- a/src/coreclr/src/pal/src/init/pal.cpp
+++ b/src/coreclr/src/pal/src/init/pal.cpp
@@ -359,18 +359,32 @@ Initialize(
         gPID = getpid();
         gSID = getsid(gPID);
 
+        // Initialize the thread local storage  
+        if (FALSE == TLSInitialize())
+        {
+            palError = ERROR_PALINIT_TLS;
+            goto done;
+        }
+
+        // Initialize debug channel settings before anything else.
+        if (FALSE == DBG_init_channels())
+        {
+            palError = ERROR_PALINIT_DBG_CHANNELS;
+            goto CLEANUP0a;
+        }
+
         // The gSharedFilesPath is allocated dynamically so its destructor does not get
         // called unexpectedly during cleanup
         gSharedFilesPath = InternalNew<PathCharString>();
         if (gSharedFilesPath == nullptr)
         {
             SetLastError(ERROR_NOT_ENOUGH_MEMORY);
-            goto done;
+            goto CLEANUP0a;
         }
 
         if (INIT_SharedFilesPath() == FALSE)
         {
-            goto done;
+            goto CLEANUP0a;
         }
 
         fFirstTimeInit = true;
@@ -400,13 +414,6 @@ Initialize(
         }
 #endif // FEATURE_ENABLE_NO_ADDRESS_SPACE_RANDOMIZATION
 
-        // Initialize the TLS lookaside cache
-        if (FALSE == TLSInitialize())
-        {
-            palError = ERROR_PALINIT_TLS;
-            goto done;
-        }
-
         InitializeCGroup();
 
         // Initialize the environment.
@@ -416,15 +423,6 @@ Initialize(
             goto CLEANUP0;
         }
 
-        // Initialize debug channel settings before anything else.
-        // This depends on the environment, so it must come after
-        // EnvironInitialize.
-        if (FALSE == DBG_init_channels())
-        {
-            palError = ERROR_PALINIT_DBG_CHANNELS;
-            goto CLEANUP0;
-        }
-
         if (!INIT_IncreaseDescriptorLimit())
         {
             ERROR("Unable to increase the file descriptor limit!\n");
@@ -726,6 +724,7 @@ CLEANUP1:
     SHMCleanup();
 CLEANUP0:
     CleanupCGroup();
+CLEANUP0a:
     TLSCleanup();
     ERROR("PAL_Initialize failed\n");
     SetLastError(palError);
diff --git a/src/coreclr/src/pal/src/misc/dbgmsg.cpp b/src/coreclr/src/pal/src/misc/dbgmsg.cpp
index 3a32b10..609eea1 100644
--- a/src/coreclr/src/pal/src/misc/dbgmsg.cpp
+++ b/src/coreclr/src/pal/src/misc/dbgmsg.cpp
@@ -157,8 +157,6 @@ BOOL DBG_init_channels(void)
     DWORD flag_mask = 0;
     int ret;
 
-    InternalInitializeCriticalSection(&fprintf_crit_section);
-
     /* output only asserts by default [only affects no-vararg-support case; if
        we have varargs, these flags aren't even checked for ASSERTs] */
     for(i=0;i<DCI_LAST;i++)
@@ -166,7 +164,11 @@ BOOL DBG_init_channels(void)
 
     /* parse PAL_DBG_CHANNELS environment variable */
 
-    env_string = EnvironGetenv(ENV_CHANNELS);
+    env_string = getenv(ENV_CHANNELS);
+    if (env_string != NULL)
+    {
+        env_string = strdup(env_string);
+    }
     env_pcache = env_workstring = env_string;
 
     while(env_workstring)
@@ -299,10 +301,10 @@ BOOL DBG_init_channels(void)
         }
         /* done processing this entry; on to the next. */
     }
-    PAL_free(env_pcache);
+    free(env_pcache);
 
     /* select output file */
-    env_string = EnvironGetenv(ENV_FILE);
+    env_string = getenv(ENV_FILE);
     if(env_string && *env_string!='\0')
     {
         if(!strcmp(env_string, "stderr"))
@@ -332,13 +334,8 @@ BOOL DBG_init_channels(void)
         output_file = stderr; /* output to stderr by default */
     }
 
-    if(env_string)
-    {
-        PAL_free(env_string);
-    }
-
     /* see if we need to disable assertions */
-    env_string = EnvironGetenv(ENV_ASSERTS);
+    env_string = getenv(ENV_ASSERTS);
     if(env_string && 0 == strcmp(env_string,"1"))
     {
         g_Dbg_asserts_enabled = FALSE;
@@ -348,17 +345,11 @@ BOOL DBG_init_channels(void)
         g_Dbg_asserts_enabled = TRUE;
     }
 
-    if(env_string)
-    {
-        PAL_free(env_string);
-    }
-
     /* select ENTRY level limitation */
-    env_string = EnvironGetenv(ENV_ENTRY_LEVELS);
+    env_string = getenv(ENV_ENTRY_LEVELS);
     if(env_string)
     {
         max_entry_level = atoi(env_string);
-        PAL_free(env_string);
     }
     else
     {
@@ -372,11 +363,12 @@ BOOL DBG_init_channels(void)
         {
             fprintf(stderr, "ERROR : pthread_key_create() failed error:%d (%s)\n",
                    ret, strerror(ret));
-            DeleteCriticalSection(&fprintf_crit_section);;
             return FALSE;
         }
     }
 
+    InternalInitializeCriticalSection(&fprintf_crit_section);
+
     return TRUE;
 }
 
-- 
2.7.4