[libcxx-commits] [PATCH] D158823: [libc++][hardening] Add back the safe mode.

[Nico Weber via Phabricator via libcxx-commits]

thakis added a comment.

This looks perfect for us, thanks!



================
Comment at: libcxx/CMakeLists.txt:53
 option(LIBCXX_ENABLE_STATIC "Build libc++ as a static library." ON)
 option(LIBCXX_ENABLE_FILESYSTEM
   "Whether to include support for parts of the library that rely on a filesystem being
----------------
var-const wrote:
> Mordante wrote:
> > ldionne wrote:
> > > ldionne wrote:
> > > > @thakis We brainstormed on some names here:
> > > > 
> > > > ```
> > > > _LIBCPP_ENABLE_HARDENED_PLUS_MODE
> > > > _LIBCPP_ENABLE_EXTENDED_HARDENED_MODE
> > > > _LIBCPP_ENABLE_STRONG_HARDENED_MODE
> > > > _LIBCPP_ENABLE_STRICT_MODE
> > > > _LIBCPP_ENABLE_PARANOID_MODE
> > > > _LIBCPP_ENABLE_FORTIFIED_MODE
> > > > _LIBCPP_ENABLE_SAFE_MODE
> > > > ```
> > > > 
> > > > Do you have any thoughts? Our thoughts so far:
> > > > 
> > > > ```
> > > > HARDENED_PLUS, EXTENDED_HARDENED, STRONG_HARDENED // those are kind of heavyweight names
> > > > PARANOID // has negative connotation and doesn't make it clear whether it is stronger than DEBUG
> > > > STRICT // could be confused with the notion of not having non-standard extensions
> > > > FORTIFIED // not clear whether it is stronger than HARDENED or not
> > > > SAFE // our current preference
> > > > ```
> > > > 
> > > > In fact, before LLVM 17 we had something called the `SAFE` mode, and I think what we discovered with Chromium's use case is that it still had its place. Hence, I think what we should do is call this the `SAFE` mode, backport this change, and rework the way we announced our 17 release notes not to say that we "replaced' the safe mode, but instead that we added new modes and that we changed how the safe mode is enabled. This is IMO a superior design and a superior way of rolling it out based on our experience so far.
> > > @Mordante Those are the other names we considered.
> > Thanks. If this list SAFE sounds the best to me too.
> @Mordante Thank you for the feedback! We spent quite a bit of time on this but unfortunately I don't have a "slam dunk" suggestion. FWIW, I'll outline my reasoning:
> - The first presumption is that we'd rather keep the current naming of "hardened" and "debug" (debug in particular seems like a good descriptive name);
> - In that case, the new mode would be in-between "hardened" and "debug" modes, and two broad alternatives are to either call it the "hardened-something" mode (to stress that it's an extension of the hardened mode, e.g. "hardened extended mode") or by a single unrelated word. The former approach seemed to produce heavyweight, cumbersome names. The problem with the latter approach is that it's hard to find a single word that makes it obvious that this mode provides more guarantees than the hardened mode.
> - My preference for "safe" is partially because it focuses on the (desired) result, not how it is achieved (I think that "hardened" vs "safe" is a little like "optimized" vs "fast"), but in large part because we already have shipped the "safe" mode. I honestly doubt it's possible to find a single word that would unambiguously convey that this mode is stronger than the hardened mode, but "safe" has precedent in its favor, so to speak.
As long as it's documented, any of those work for me. Naming tends to be a bikeshed. Here's some more ideas (for the cmake arg), but go with what you like best:

some, most, debug
safe, strict, debug
basic, thorough, debug
minimal, robust, debug
fast, conservative, debug

i.e. maybe renaming the "hardened" level could make the progression clearer. If you want to keep "hardened" as base level, maybe "thorough" or "conservative" are still contenders. But as I said, I don't mind "safe".




================
Comment at: libcxx/include/__config:319
+#    define _LIBCPP_ASSERT_COMPATIBLE_ALLOCATOR(expression, message)     _LIBCPP_ASSERT(expression, message)
+// TODO(hardening): don't enable uncategorized assertions in the safe mode.
+#    define _LIBCPP_ASSERT_UNCATEGORIZED(expression, message)            _LIBCPP_ASSERT(expression, message)
----------------
TODO when? After a security review of these asserts? Something else? Do you have a rough time frame when this might happen?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158823/new/

https://reviews.llvm.org/D158823