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

Mon Aug 28 17:51:39 PDT 2023

var-const added a comment.

In D158823#4621784 <https://reviews.llvm.org/D158823#4621784>, @crtrott wrote:

> I believe this 3-level thing is in general a good idea, and in principle the words "hardened < safe < debug" fell ok-ish to me in terms of expectations how much gets checked.
> However, in light of a 3-level mode I may want to rethink for example the asserts in mdspan, and tailor better what is enabled in hardened, vs safe mode. I think the current choices were good in a binary world, in this more nuanced approach I am not sure.

Thanks a lot for your comments! Re. `mdspan` -- IIRC, we classified most of the assertions in `mdspan` as `valid-element-access`, which would enable them in all modes. The only exceptions are:

- `layout_left/right::mapping` checks -- IMO those should be enabled in the safe mode;
- `mdspan: size() is not representable as size_type` -- I also think this should be on in the safe mode.

(FWIW, my current informal criteria for the safe mode is "any check as long as it's for UB triggered by user input and not too expensive". With that mindset, none of the checks I remember in `mdspan` strike me as debug-only -- i.e., all of them make sense in the safe mode as well)

================
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
----------------
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.

================
Comment at: libcxx/docs/ReleaseNotes/17.rst:137-140
 - The "safe" mode is replaced by the hardened mode in this release. The ``LIBCXX_ENABLE_ASSERTIONS`` CMake variable is
   deprecated and setting it will trigger an error; use ``LIBCXX_HARDENING_MODE`` instead. Similarly, the
   ``_LIBCPP_ENABLE_ASSERTIONS`` macro is deprecated and setting it to ``1`` now enables the hardened mode. See
   ``libcxx/docs/Hardening.rst`` for more details.
----------------
ldionne wrote:
> 
Applied (with a few tweaks, please let me know if they look reasonable to you).

================
Comment at: libcxx/include/__config:287-289
+#  if (_LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_SAFE_MODE) ||                                                    \
+      (_LIBCPP_ENABLE_HARDENED_MODE && _LIBCPP_ENABLE_DEBUG_MODE) ||                                                   \
+      (_LIBCPP_ENABLE_SAFE_MODE && _LIBCPP_ENABLE_DEBUG_MODE)
----------------
ldionne wrote:
> No strong preference, but we could also do `#if HARDENED_MODE + SAFE_MODE + DEBUG_MODE > 1`.
Thanks! I think it's better (I don't really expect another mode at this point, but if that ever happens, the new form of this check is much easier to modify).

================
Comment at: libcxx/utils/libcxx/test/params.py:301-303
                 AddCompileFlag("-D_LIBCPP_ENABLE_HARDENED_MODE=1") if hardening_mode == "hardened" else None,
+                AddCompileFlag("-D_LIBCPP_ENABLE_SAFE_MODE=1")     if hardening_mode == "safe" else None,
                 AddCompileFlag("-D_LIBCPP_ENABLE_DEBUG_MODE=1")    if hardening_mode == "debug" else None,
----------------
Mordante wrote:
> Mordante wrote:
> > ldionne wrote:
> > > Mordante wrote:
> > > > I start to feel slightly uncomfortable with these names. To me they are not very descriptive and they are now 4 options. I don't directly have better suggestions, but I think we should spend a bit of time on this.
> > > > 
> > > > For example, which is more expensive "safe" or "hardened"?
> > > I agree, this is definitely not perfect. This is user facing too, so we need to find something good.
> > > 
> > > We thought that `safe` was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than `hardened`, but maybe it isn't. I'll CC you on the comment above that discusses naming.
> > Thanks. As said I don't have a better suggestion either.
> > I agree, this is definitely not perfect. This is user facing too, so we need to find something good.
> > 
> > We thought that `safe` was good given that it is the name we used for this mode historically (in LLVM 15 and LLVM 16). We also thought that it was "reasonably" clear that it was more expensive than `hardened`, but maybe it isn't. I'll CC you on the comment above that discusses naming.
> 
> Based on this information and Discord I think we should go with safe. I still think it's not great, but it's consistent with what we did in the past.
(I'm marking these comments as "Done" just so that we can keep this conversation in one thread)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158823