chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.38k stars 467 forks source link

win: Explore alternatives to use_custom_libcxx=false for cef_sandbox #3824

Open magreenblatt opened 2 weeks ago

magreenblatt commented 2 weeks ago

Is your feature request related to a problem? Please describe. Linking the cef_sandbox static library is required for using the sandbox on Windows. Chromium's custom bundled libc++ version must be disabled (set use_custom_libcxx=false) when building cef_sandbox in order to link with the MSVC stdlib that is compatible with third-party apps.

Chromium is planning to remove the MSVC stdlib compile/link option for reasons. This will break the way that we currently build/utilize cef_sandbox on Windows.

For the reasons described in the document, including that Chromium is going to fail to compile in the future with MSVC stdlib (similar reasons as with libstdc++), support for libraries other than libc++ is going away.

This leaves us with the following options:

  1. Revert/disable the related Chromium changes. This is likely to be unsustainable as a long-term solution.
  2. Remove the cef_sandbox static linking requirement. It's unclear if/how the Windows sandbox would work in this case.
  3. Distribute Chromium's libc++ with cef_sandbox. This is likely a non-starter as we would also need to distribute the libc++ headers and all third-party apps would need to use them instead of the platform/toolchain default version [1].
  4. Distribute cef_sandbox in source code form (similar to libcef_dll_wrapper) to be built as part of the third-party app. This is likely a non-starter as we can't easily extract the required source files or build config from Chromium.

[1] Apps that already build against a "new enough" version of clang/llvm libc++ might be fine in this case.

magreenblatt commented 2 weeks ago

Remove the cef_sandbox static linking requirement. It's unclear if/how the Windows sandbox would work in this case.

From the Windows sandbox docs:

The interceptions (also known as hooks) are how Windows API calls are forwarded via the sandbox IPC to the broker.

This is the part that requires static linking of cef_sandbox on Windows. The interception needs to run in the main executable (not libcef.dll) to properly hook WinAPI functions.

From a related thread:

The sandbox code cannot be used as-is to sandbox a random executable. The sandbox library has to be used in both the broker AND the target process, and it has to reside in the main executable, not in a DLL.

Even the interception code by itself relies on the presence of the library compiled into the target executable.

That thread also contains related technical implementation details that are still valid today.

danakj commented 1 week ago

Asking some questions to understand more:

magreenblatt commented 1 week ago

@danakj Thanks for the follow up!

This bug does not talk about any other CEF libraries. Is the rest of CEF distributed as source and built by the users of CEF?

We distribute CEF + Chromium as a binary distribution that includes libcef.dll (compiled CEF + Chromium code), CEF header files (C and C++), and related resource files (.bin, .pak, etc, same as Chromium). We build libcef.dll as part of the Chromium build (//cef targets added to Chromium's top-level .gn file), utilizing the same Chromium toolchain. The libcef.dll exposes a C ABI only, so no Chromium or C++ symbols are exported across the dll boundary. This allows third-party applications to build with platform toolchains (MSVC stdlib in this case) and utilize CEF without requiring Chromium-specific toolchains or header files.

This bug talks about cef_sandbox. It is distributed as a .lib and header files?

The cef_sandbox is basically just an entry point wrapper for the Chromium //sandbox target. It's distributed as a .lib (binary contents here) and accessed via a single CEF header file (CEF-side implementation here). With this approach there are linker dependencies on Chromium (.objs from //sandbox, //base, etc, in cef_sandbox.lib) but no source or header dependencies.

Why is cef_sandbox different?

Because of the Windows sandbox usage requirements described above.

The libc++ headers are present in //buildtools/third_party/libc++. They would need to be used by an app building/linking chromium (CEF, I think?) source code as well. Can they not point at them for both CEF and cef_sandbox usage?

These headers are used when compiling libcef.dll, but not when building or using cef_sandbox (because we set use_custom_libcxx=false). We build the cef_sandbox target (including //sandbox, //base, etc, dependencies) separately from the rest of CEF/Chromium using a different GN configuration. Note that we're only building a small subset of Chromium targets with use_custom_libcxx=false for this use case.

"This is likely a non-starter as we can't easily extract the required source files or build config from Chromium." The cef_sandbox is not a build target in Chromium, what makes this so difficult?

We would need to extract all source files (.cc and .h) that are used by the cef_sandbox target and its Chromium dependencies (//sandbox, //base, etc). You can see the complete list of dependent build artifacts here. The //base target, for example, changes frequently so we wouldn't want to do this as a manual process. Automatically extracting those source files from the Chromium configuration and generating the necessary build configuration for CMake or Bazel (for use by third-party apps) would be challenging. Also, those Chromium files would still need to build successfully with the MSVC stdlib, something that we're likely to lose with the removal of use_custom_libcxx=false support. Note that MSVC support is currently community-supported and we've submitted a number of CLs (1, 2, 3, etc) over the years to keep it working.

danakj commented 1 week ago

I see, you're literally building Chromium code against a different stdlib. With our plans for deep and ergonomic interop, that's going to stop compiling (or break entirely at runtime if you lie about which headers will be used to compile with).

Note that we're only building a small subset of Chromium targets with use_custom_libcxx=false for this use case.

Yeah, unfortunately some of the most important places where interop will be required and active though (like //base). As such, distributing the //sandbox code and expecting clients to build it against their own stdlib is also not going to work. The interop tool will generate different bindings, and code in //sandbox and its deps that is using those bindings will fail to build.

Clients will really need to use //buildtools/third_party/libc++ to build any part of Chromium in the future.

magreenblatt commented 1 week ago

@danakj Thanks, that background is really helpful. Maybe there's a better way to implement the sandbox interception on Windows today (Windows 10+ world) so that we can eliminate the cef_sandbox.lib linking requirement all-together? Who would be a good person inside Chromium security to discuss that with?

danakj commented 1 week ago

I asked around and @wfh-chromium suggested that you file a crbug.com about this, as it should be possible to not require linking the sandbox code into the main exe. It just hasn't been prioritized yet, but if there's a bug about it then they can sort it into their planning.

magreenblatt commented 1 week ago

I asked around and @wfh-chromium suggested that you file a crbug.com about this, as it should be possible to not require linking the sandbox code into the main exe.

Awesome! I filed https://issues.chromium.org/issues/378046068

magreenblatt commented 6 days ago

This warning now displays when generating the cef_sandbox target in M132+:

*********************************************************************
WARNING: Support for linking against a C++ standard library other
  than the one in-tree (buildtools/third_party/libc++) is deprecated
  and support for this will end. We plan to remove this option in
  M138.
*********************************************************************