Statically linking CRT for aws-lc-fips-sys

[samin-cf]

Problem:

It would greatly simplify some of our Windows build and installer process if aws-lc-fips-sys supported statically-linking CRT with the /MT flag, similar to the aws-lc-sys crate.

In detail, we are distributing aws-lc-fips DLLs that some of the binaries depend on, but they fail to run on systems where VCRUNTIME140.dll is not present.

Attempted Solution:

I did try setting cmake_cfg.static_crt(true) in aws-lc-fips-sys's cmake_builder.rs, but it didn't make a difference. With cargo build -vvv -p aws-lc-fips-sys, I saw

[aws-lc-fips-sys 0.12.11] cl : Command line warning D9025 : overriding '/MT' with '/MD'
[aws-lc-fips-sys 0.12.11] cl : Command line warning D9002 : ignoring unknown option '-fno-function-sections'
[aws-lc-fips-sys 0.12.11] cl : Command line warning D9002 : ignoring unknown option '-fno-data-sections

Something seems to be overriding the /MT flag

[justsmth]

I did a little research into this which resulted in a PR I posted upstream.

As you know, our FIPS build on Windows currently only supports building shared (DLL) libraries. It's important to be aware that there are fundamental problems that can occur when using static CRT when linking a shared library.

For example, if a class is allocated in one DLL but passed to and deleted by another, which CRT deallocator is used? The errors caused can range from the subtle to the immediately fatal, and therefore direct transfer of such resources is discouraged. ... To ensure that all components use the same DLL version of the CRT, build them by using the /MD option, and use the same compiler toolset and property settings.

In aws-lc-rs, there are quite a few objects allocated within AWS-LC that could be freed by Rust logic. (Actually, freeing memory for those values I linked to might be ok since we're calling back into the AWS-LC library to free them. But I'm not confident that we never transfer "ownership" of allocated memory across the API boundary. It seems likely that this would result in a memory leak or other runtime failure.)

If you're confident and would like to give it a shot anyways, it might be possible to get this to work by setting CMAKE_TOOLCHAIN_FILE in the environment and configuring specific compile flags to be passed in? I'd be interested if you can find a way to make it work.

[samin-cf]

Interesting, thank you for looking into this!

To workaround the installer issue, we have been using a custom fork of aws-lc-rs v1.7.1 with aws-lc-fips-sys v0.12.8 where in aws-lc-fips-sys/CMakeLists.txt, we explicitly set the cmake policy to CMP0091 and set CMAKE_MSVC_RUNTIME_LIBRARY to MultiThreaded$<$<CONFIG:Debug>:Debug>. So far, this has been fine (no crashes or memory leaks observed), and we haven't experienced any issues mentioned in that Microsoft doc. Of course, this doesn't mean the problems aren't real, but just providing some reference.

We now need to update the version of aws-lc-rs to pull in new features and would like to avoid maintaining a custom fork if possible!

Curious to know, would any potential issues with static CRT be applicable only for aws-lc-fips-sys or would they also apply to aws-lc-sys?

[justsmth]

We now need to update the version of aws-lc-rs to pull in new features and would like to avoid maintaining a custom fork if possible!

Have you tried creating a CMake "toolchain" configuration file to pass in? I think this would allow setting "CMP0091" and "CMAKE_MSVC_RUNTIME_LIBRARY". This might be more maintainable in the long term.

Curious to know, would any potential issues with static CRT be applicable only for aws-lc-fips-sys or would they also apply to aws-lc-sys?

I can't think of any issues related to "static CRT" that would different between aws-lc-sys (from AWS-LC's "main" branch) and aws-lc-fips-sys. (The branch of AWS-LC currently used by aws-lc-fips-sys is essentially just a snapshot of our main branch taken sometime around the date of "2022-11-02", with several patches applied to address important issues that have come up.) Of course, the main difference is that on Windows the FIPS build is a shared library and non-FIPS is static.

[samin-cf]

No, I haven't given the toolchain configuration file a try yet but will do so and provide the results!

[justsmth]

I'm looking more closely at the places were we transfer memory "ownership" across the API boundary.

From the places I've checked (such as here or here), we are using OPENSSL_malloc when allocating memory that we will transfer into an AWS-LC struct. We also don't use mem::forget or Box::leak anywhere, so I'm feeling more confident about this being OK. But we still need to verify that our automated tests are able to catch such an issue if one were introduced.

[samin-cf]

Finally got a chance to try the toolchain approach, and I have kind of got it working

This is what I initially had in the toolchain file:

if (BUILD_SHARED_LIBS AND FIPS)
    if(POLICY CMP0091)
        cmake_policy(SET CMP0091 NEW)
    endif()

    message(STATUS "Setting link to /MTd")
    set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreadedDebug")
endif

In the cargo build output, I can see the "Setting link to /MTd" message, but the CMAKE_MSVC_RUNTIME_LIBRARY variable isn't having any effect (tried MultiThreaded and MultiThreaded$<$:Debug> as well).

I assume this is because of how cc-rs and cmake-rs handle (or rather lack of) the MSVC runtime configuration. They seem to be controlling the MSVC runtime configuration directly using the C[XX] flags.

So, modifying the flags directly seems to work:

# To prevent multiple inclusions of this script
include_guard(GLOBAL)

if (BUILD_SHARED_LIBS AND FIPS AND CMAKE_PROJECT_NAME STREQUAL "AWS_LC_RUST")
  include(CMakeInitializeConfigs)

  # Override the cmake_initialize_per_config_variable function to replace all instances
  # of /MD with /MT and -MD with -MT for the CMAKE_(C|CXX)_FLAGS_<CONFIG> variables
  function(cmake_initialize_per_config_variable _PREFIX _DOCSTRING)
    if (_PREFIX MATCHES "CMAKE_(C|CXX)_FLAGS")

      # Loop through each configuration-specific flag initialization
      foreach (config 
        ${_PREFIX}_DEBUG_INIT 
        ${_PREFIX}_RELEASE_INIT 
        ${_PREFIX}_RELWITHDEBINFO_INIT 
        ${_PREFIX}_MINSIZEREL_INIT)

        # Print the configuration and its initial value
        #message(STATUS "Initial ${config}: ${${config}}")

        # Replace "/MD" with "/MT" in the configuration-specific flags
        string(REPLACE "/MD" "/MT" ${config} "${${config}}")

        # Print the modified value
        #message(STATUS "Modified ${config}: ${${config}}")
      endforeach()
    endif()

    # Call the original _cmake_initialize_per_config_variable function
    _cmake_initialize_per_config_variable(${ARGV})
  endfunction()

  # Replace -MD with -MT in the base C and C++ base compiler flags

  if (DEFINED CMAKE_C_FLAGS)
      message(STATUS "Initial CMAKE_C_FLAGS: ${CMAKE_C_FLAGS}")
      string(REPLACE "-MD" "-MT" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")
      message(STATUS "Modified CMAKE_C_FLAGS: ${CMAKE_C_FLAGS}")
  endif()

  if (DEFINED CMAKE_CXX_FLAGS)
      message(STATUS "Initial CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}")
      string(REPLACE "-MD" "-MT" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
      message(STATUS "Modified CMAKE_CXX_FLAGS: ${CMAKE_CXX_FLAGS}")
  endif()

  # Ensure the modified flags are used
  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS}" CACHE STRING "Modified C compiler flags" FORCE)
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}" CACHE STRING "Modified C++ compiler flags" FORCE)
endif()

This seems to work based on dumpbin output.

For our use case, however, it's looking like globally setting the CMAKE_GENERATOR to Ninja and CMAKE_TOOLCHAIN_FILE in the project's .cargo/config.toml breaks the build for other dependencies, so the toolchain approach may not be feasible for us (unless Cargo has a way to set those variables only for aws-lc-rs).

Anyhow, this isn't really a problem with aws-lc-rs itself.

[justsmth]

We recently merged in a related change for AWS-LC so we better support CMAKE_MSVC_RUNTIME_LIBRARY. This upstream change will be part of our upcoming aws-lc-rs release. I'm curious whether that change might allow your initial toolchain setup to work?