Clang/GCC compiler settings update [SECURITY] (#7358)

- Add the following compilation flags for clang
  -fpie
  -fstack-protector-all
  -fstack-check (non-release)

- Add the following compilation flags for gcc
  -fpie
  -fstack-protector-all

- Fix -Wunused-result errors from above compilation flag updates

- Add _FORTIFY_SOURCE=2 to GCC DEFINES

Signed-off-by: Steve Pham <82231385+spham-amzn@users.noreply.github.com>

Code/Editor/Util/ImageUtil.cpp

@@ -143,7 +143,8 @@ bool CImageUtil::LoadPGM(const QString& fileName, CImageEx& image)
     fseek(file, 0, SEEK_SET);
 
     char* str = new char[fileSize];
-    fread(str, fileSize, 1, file);
+
+    [[maybe_unused]] auto bytesRead = fread(str, fileSize, 1, file);
 
     [[maybe_unused]] char* nextToken = nullptr;
     token = azstrtok(str, 0, seps, &nextToken);

Code/Framework/AzCore/Tests/Platform/Android/Tests/UtilsTests_Android.cpp

@@ -69,9 +69,11 @@ namespace UnitTest
         // Note that ConvertToAbsolutePath will perform a realpath on the result. The result of AZ::Utils::GetExecutableDirectory
         // uses AZ::Android::AndroidEnv::Get()->GetAppPrivateStoragePath() which will retrieve the storage path, but that path could
         // be symlinked, so we need to perform a real path on it before comparison
-        char realExecutableDirectory[AZ::IO::MaxPathLength];
-        ASSERT_TRUE(realpath(executableDirectory, realExecutableDirectory));
-
+        char* realExecutableDirectory = realpath(executableDirectory, nullptr);
+        ASSERT_NE(realExecutableDirectory, nullptr);
+        
         EXPECT_STRCASEEQ(realExecutableDirectory, absolutePath->c_str());
+
+        free(realExecutableDirectory);
     }
 }

Code/Framework/AzFramework/AzFramework/Archive/ZipDirCache.cpp

@@ -383,7 +383,7 @@ namespace AZ::IO::ZipDir
             if (!AZ::IO::FileIOBase::GetDirectInstance()->Write(m_fileHandle, ptr, sizeToWrite))
             {
                 char error[1024];
-                azstrerror_s(error, AZ_ARRAY_SIZE(error), errno);
+                [[maybe_unused]] auto azStrErrorResult = azstrerror_s(error, AZ_ARRAY_SIZE(error), errno);
                 AZ_Warning("Archive", false, "Cannot write to zip file!! error = (%d): %s", errno, error);
                 return ZD_ERROR_IO_FAILED;
             }
@@ -531,7 +531,7 @@ namespace AZ::IO::ZipDir
         if (!WriteCompressedData((uint8_t*)pUncompressed, nSegmentSize, encrypt))
         {
             char error[1024];
-            azstrerror_s(error, AZ_ARRAY_SIZE(error), errno);
+            [[maybe_unused]] auto azStrErrorResult = azstrerror_s(error, AZ_ARRAY_SIZE(error), errno);
             AZ_Warning("Archive", false, "Cannot write to zip file!! error = (%d): %s", errno, error);
             return ZD_ERROR_IO_FAILED;
         }

Code/Framework/AzFramework/Platform/Linux/AzFramework/Process/ProcessWatcher_Linux.cpp

@@ -120,7 +120,7 @@ namespace AzFramework
                 int res = chdir(processLaunchInfo.m_workingDirectory.c_str());
                 if (res != 0)
                 {
-                    write(errorPipe[1], &errno, sizeof(int));
+                    [[maybe_unused]] auto writeResult = write(errorPipe[1], &errno, sizeof(int));
                     // We *have* to _exit as we are the child process and simply
                     // returning at this point would mean we would start running
                     // the code from our parent process and that will just wreck
@@ -132,15 +132,19 @@ namespace AzFramework
             switch (processLaunchInfo.m_processPriority)
             {
                 case PROCESSPRIORITY_BELOWNORMAL:
-                    nice(1);
+                {
+                    [[maybe_unused]] auto niceResult = nice(1);
                     // also reduce disk impact:
                     // setiopolicy_np(IOPOL_TYPE_DISK, IOPOL_SCOPE_PROCESS, IOPOL_UTILITY);
                     break;
+                }
                 case PROCESSPRIORITY_IDLE:
-                    nice(20);
+                {
+                    [[maybe_unused]] auto niceResult = nice(20);
                     // also reduce disk impact:
                     // setiopolicy_np(IOPOL_TYPE_DISK, IOPOL_SCOPE_PROCESS, IOPOL_THROTTLE);
                     break;
+                }
             }
 
             startupInfo.SetupHandlesForChildProcess();
@@ -153,7 +157,7 @@ namespace AzFramework
             // to stop it from continuing to run as a clone of the parent.
             // Communicate the error code back to the parent via a pipe for the
             // parent to read.
-            write(errorPipe[1], &errval, sizeof(errval));
+            [[maybe_unused]] auto writeResult = write(errorPipe[1], &errval, sizeof(errval));
 
             _exit(0);
         }
@@ -317,7 +321,7 @@ namespace AzFramework
 
         // Set up a pipe to communicate the error code from the subprocess's execvpe call
         AZStd::array<int, 2> childErrorPipeFds{};
-        pipe(childErrorPipeFds.data());
+        [[maybe_unused]] auto pipeResult = pipe(childErrorPipeFds.data());
 
         // This configures the write end of the pipe to close on calls to `exec`
         fcntl(childErrorPipeFds[1], F_SETFD, fcntl(childErrorPipeFds[1], F_GETFD) | FD_CLOEXEC);

Code/Framework/AzNetworking/Platform/Common/UnixLike/AzNetworking/Utilities/NetworkCommon_UnixLike.cpp

@@ -71,7 +71,7 @@ namespace AzNetworking
         const char* GetNetworkErrorDesc(int32_t errorCode)
         {
             static AZ_THREAD_LOCAL char buffer[1024];
-            strerror_r(errorCode, buffer, sizeof(buffer));
+            [[maybe_unused]] auto strErrorResult = strerror_r(errorCode, buffer, sizeof(buffer));
             return buffer;
         }
     }

Code/Tools/AzTestRunner/Platform/Common/platform_host_posix.cpp

@@ -13,7 +13,7 @@ namespace AzTestRunner
 {
     void set_quiet_mode()
     {
-        freopen("/dev/null", "a", stdout);
+        [[maybe_unused]] auto freopenResult = freopen("/dev/null", "a", stdout);
     }
 
     const char* get_current_working_directory()
@@ -24,7 +24,7 @@ namespace AzTestRunner
 
     void pause_on_completion()
     {
-        system("pause");
+        [[maybe_unused]] auto systemResult = system("pause");
     }
 
 }

Gems/AtomTressFX/External/Code/src/TressFX/TressFXAsset.cpp

@@ -282,8 +282,8 @@ namespace AMD
         TressFXTFXFileHeader header = {};
 
         // read the header
-        EI_Seek(ioObject, 0); // make sure the stream pos is at the beginning. 
-        EI_Read((void*)&header, sizeof(TressFXTFXFileHeader), ioObject);
+        [[maybe_unused]] auto eiSeekResult = EI_Seek(ioObject, 0); // make sure the stream pos is at the beginning. 
+        [[maybe_unused]] auto eiReadResult = EI_Read((void*)&header, sizeof(TressFXTFXFileHeader), ioObject);
 
         // If the tfx version is lower than the current major version, exit. 
         if (header.version < AMD_TRESSFX_VERSION_MAJOR)
@@ -317,8 +317,8 @@ namespace AMD
         m_positions.resize(m_numTotalVertices); // size of m_positions = number of total vertices * sizeo of each position vector. 
 
                                                 // Read position data from the io stream. 
-        EI_Seek(ioObject, header.offsetVertexPosition);
-        EI_Read((void*)m_positions.data(), numStrandsInFile * m_numVerticesPerStrand * sizeof(AMD::float4), ioObject); // note that the position data in io stream contains only guide hairs. If we call GenerateFollowHairs
+        eiSeekResult = EI_Seek(ioObject, header.offsetVertexPosition);
+        eiReadResult = EI_Read((void*)m_positions.data(), numStrandsInFile * m_numVerticesPerStrand * sizeof(AMD::float4), ioObject); // note that the position data in io stream contains only guide hairs. If we call GenerateFollowHairs
                                                                                                                        // to generate follow hairs, m_positions will be re-allocated. 
 
                                                                                                                        // We need to make up some strands to fill up the buffer because the number of strands from stream is not necessarily multile of thread size. 
@@ -335,11 +335,11 @@ namespace AMD
         }
 
         // Read strand UVs
-        EI_Seek(ioObject, header.offsetStrandUV);
+        eiSeekResult = EI_Seek(ioObject, header.offsetStrandUV);
         m_strandUV.resize(m_numTotalStrands); // If we call GenerateFollowHairs to generate follow hairs, 
                                               // m_strandUV will be re-allocated. 
 
-        EI_Read((void*)m_strandUV.data(), numStrandsInFile * sizeof(AMD::float2), ioObject);
+        eiReadResult = EI_Read((void*)m_strandUV.data(), numStrandsInFile * sizeof(AMD::float2), ioObject);
 
         // Fill up the last empty space
         AMD::int32 indexLastStrand = (numStrandsInFile - 1);
@@ -683,21 +683,21 @@ namespace AMD
     void TressFXAsset::GetBonesNames(FILE* ioObject, std::vector<std::string>& boneNames)
     {
         AMD::int32 numOfBones = 0;
-        EI_Seek(ioObject, 0);
-        EI_Read((void*)&numOfBones, sizeof(AMD::int32), ioObject);
+        [[maybe_unused]] auto eiSeekResult = EI_Seek(ioObject, 0);
+        [[maybe_unused]] auto eiReadResult = EI_Read((void*)&numOfBones, sizeof(AMD::int32), ioObject);
 
         //    boneNames.reserve(numOfBones);
         boneNames.resize(numOfBones);
         for (int i = 0; i < numOfBones; i++)
         {
             int boneIndex;
-            EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
+            eiReadResult = EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
 
             AMD::int32 charLen = 0;
-            EI_Read((char*)&charLen, sizeof(AMD::int32), ioObject); // character length includes null termination already.
+            eiReadResult = EI_Read((char*)&charLen, sizeof(AMD::int32), ioObject); // character length includes null termination already.
 
             char boneName[128];
-            EI_Read(boneName, sizeof(char) * charLen, ioObject);
+            eiReadResult = EI_Read(boneName, sizeof(char) * charLen, ioObject);
             boneName[charLen] = '\0';   // adding 0 termination to be on the safe side.
             boneNames[i] = std::string(boneName);
         }
@@ -730,8 +730,8 @@ namespace AMD
         m_boneSkinningData.resize(0);
 
         AMD::int32 numOfBones = 0;
-        EI_Seek(ioObject, 0);
-        EI_Read((void*)&numOfBones, sizeof(AMD::int32), ioObject);
+        [[maybe_unused]] auto eiSeekResult = EI_Seek(ioObject, 0);
+        [[maybe_unused]] auto eiReadResult = EI_Read((void*)&numOfBones, sizeof(AMD::int32), ioObject);
 
         if (skeletonBoneIndices.size() != numOfBones)
         {
@@ -742,18 +742,18 @@ namespace AMD
         for (int i = 0; i < numOfBones; i++)
         {
             int boneIndex;
-            EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
+            eiReadResult = EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
 
             AMD::int32 charLen = 0;
-            EI_Read((char*)&charLen, sizeof(AMD::int32), ioObject); // character length includes null termination already.
+            eiReadResult = EI_Read((char*)&charLen, sizeof(AMD::int32), ioObject); // character length includes null termination already.
 
             char boneName[128];
-            EI_Read(boneName, sizeof(char) * charLen, ioObject);
+            eiReadResult = EI_Read(boneName, sizeof(char) * charLen, ioObject);
         }
 
         // Reading the number of strands 
         AMD::int32 numOfStrandsInStream = 0;
-        EI_Read((char*)&numOfStrandsInStream, sizeof(AMD::int32), ioObject);
+        eiReadResult = EI_Read((char*)&numOfStrandsInStream, sizeof(AMD::int32), ioObject);
 
         //If the number of strands from the input stream (tfxbone) is bigger than what we already know from tfx, something is wrong.
         if (m_numGuideStrands < numOfStrandsInStream)
@@ -765,15 +765,15 @@ namespace AMD
         for (int i = 0; i < numOfStrandsInStream; ++i)
         {
             AMD::int32 index = 0; // Well, we don't really use this here. 
-            EI_Read((char*)&index, sizeof(AMD::int32), ioObject);
+            eiReadResult = EI_Read((char*)&index, sizeof(AMD::int32), ioObject);
 
             for (AMD::int32 j = 0; j < TRESSFX_MAX_INFLUENTIAL_BONE_COUNT; ++j)
             {
                 AMD::int32 boneIndex;
-                EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
+                eiReadResult = EI_Read((char*)&boneIndex, sizeof(AMD::int32), ioObject);
                 assert(boneIndex >= 0);
                 skinData.boneIndex[j] = (float)skeletonBoneIndices[boneIndex]; // Change the joint index to be what the engine wants
-                EI_Read((char*)&skinData.weight[j], sizeof(AMD::real32), ioObject);
+                eiReadResult = EI_Read((char*)&skinData.weight[j], sizeof(AMD::real32), ioObject);
             }
 
 #if defined(AZ_ENABLE_TRACING)
@@ -987,4 +987,3 @@ namespace AMD
         return true;
     }
 } // namespace AMD
-

cmake/Platform/Common/Clang/Configurations_clang.cmake

@@ -9,6 +9,8 @@
 include(cmake/Platform/Common/Configurations_common.cmake)
 
 ly_append_configurations_options(
+    DEFINES
+        _FORTIFY_SOURCE=2
     COMPILATION
         -fno-exceptions
         -fvisibility=hidden
@@ -16,6 +18,8 @@ ly_append_configurations_options(
         -Wall
         -Werror
 
+        -fpie                   # Position-Independent Executables
+
         ###################
         # Disabled warnings (please do not disable any others without first consulting sig-build)
         ###################
@@ -35,17 +39,22 @@ ly_append_configurations_options(
         ###################
 
     COMPILATION_DEBUG
-        -O0 # No optimization
-        -g # debug symbols
-        -fno-inline # don't inline functions
-        -fstack-protector # Add additional checks to catch stack corruption issues
+        -O0                         # No optimization
+        -g                          # debug symbols
+        -fno-inline                 # don't inline functions
+
+        -fstack-protector-all       # Enable stack protectors for all functions
+        -fstack-check
+
     COMPILATION_PROFILE
         -O2
-        -g # debug symbols
+        -g                          # debug symbols
+
+        -fstack-protector-all       # Enable stack protectors for all functions
+        -fstack-check
+
     COMPILATION_RELEASE
         -O2
-    LINK_NON_STATIC
-        -Wl,-undefined,error
 )
 
 include(cmake/Platform/Common/TargetIncludeSystemDirectories_supported.cmake)

cmake/Platform/Common/GCC/Configurations_gcc.cmake

@@ -20,6 +20,8 @@ endif()
 
 
 ly_append_configurations_options(
+    DEFINES
+        _FORTIFY_SOURCE=2
 
     COMPILATION_C
         -fno-exceptions
@@ -27,6 +29,9 @@ ly_append_configurations_options(
         -Wall
         -Werror
 
+        -fpie                   # Position-Independent Executables
+        -fstack-protector-all   # Enable stack protectors for all functions
+
         ${LY_GCC_GCOV_FLAGS}
         ${LY_GCC_GPROF_FLAGS}
 
@@ -37,6 +42,9 @@ ly_append_configurations_options(
         -Wall
         -Werror
 
+        -fpie                   # Position-Independent Executables
+        -fstack-protector-all   # Enable stack protectors for all functions
+
         ${LY_GCC_GCOV_FLAGS}
         ${LY_GCC_GPROF_FLAGS}
 
@@ -71,12 +79,23 @@ ly_append_configurations_options(
         -O0 # No optimization
         -g # debug symbols
         -fno-inline # don't inline functions
-        -fstack-protector # Add additional checks to catch stack corruption issues
     COMPILATION_PROFILE
         -O2
         -g # debug symbols
     COMPILATION_RELEASE
         -O2
+
+    LINK_NON_STATIC
+        -Wl,-undefined,error
+        -fpie
+        -Wl,-z,relro,-z,now
+        -Wl,-z,noexecstack
+    LINK_EXE
+        -pie
+        -fpie
+        -Wl,-z,relro,-z,now
+        -Wl,-z,noexecstack
+
 )
 
 include(cmake/Platform/Common/TargetIncludeSystemDirectories_supported.cmake)

cmake/Platform/Linux/Configurations_linux.cmake

@@ -18,7 +18,18 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
         COMPILATION
             -fPIC
             -msse4.1
+        LINK_NON_STATIC
+            -Wl,-undefined,error
+            -fpie
+            -Wl,-z,relro,-z,now
+            -Wl,-z,noexecstack
+        LINK_EXE
+            -pie
+            -fpie
+            -Wl,-z,relro,-z,now
+            -Wl,-z,noexecstack
     )
+
     ly_set(CMAKE_CXX_EXTENSIONS OFF)
 elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")