std::optional

[Avi Drissman]

Right now we're using absl::optional, and I just got a review that used std::optional. It seems like quite a few uses have found their way into the tree.

Is absl::optional hardened in a way that would make a migration harder, or is it time to move to std::optional?

Avi

[dan...@chromium.org]

On Tue, Feb 15, 2022 at 1:46 PM 'Avi Drissman' via cxx <c...@chromium.org> wrote:

Right now we're using absl::optional, and I just got a review that used std::optional. It seems like quite a few uses have found their way into the tree.

Is absl::optional hardened in a way that would make a migration harder, or is it time to move to std::optional?

Yes, absl::optional will crash instead of give you UB when you try to use its value() but it's not present. std::optional is banned for this reason. Not banned hard enough I guess.

 

Avi

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAa%3DKcCVipq4TSqdx1M5Zgy6-3da0r1QTra6iL_0WGUWMw%40mail.gmail.com.

[Avi Drissman]

Where do we go from here?

- Do we make a CL switching uses in our tree to absl::?

- Do we add a presubmit (sigh) or a Tricium plugin to alert folks?

Whatever happened to the stdlib hardening switch?

[Daniel Cheng]

On Tue, 15 Feb 2022 at 10:54, 'Avi Drissman' via cxx <c...@chromium.org> wrote:

Where do we go from here?

- Do we make a CL switching uses in our tree to absl::?

Yes.

- Do we add a presubmit (sigh) or a Tricium plugin to alert folks?

Yes to the presubmit.

 

Whatever happened to the stdlib hardening switch?

Mostly lack of anyone working on it :)

Daniel

 

On Tue, Feb 15, 2022 at 1:47 PM <dan...@chromium.org> wrote:

On Tue, Feb 15, 2022 at 1:46 PM 'Avi Drissman' via cxx <c...@chromium.org> wrote:

Right now we're using absl::optional, and I just got a review that used std::optional. It seems like quite a few uses have found their way into the tree.

Is absl::optional hardened in a way that would make a migration harder, or is it time to move to std::optional?

Yes, absl::optional will crash instead of give you UB when you try to use its value() but it's not present. std::optional is banned for this reason. Not banned hard enough I guess.

 

Avi

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAa%3DKcCVipq4TSqdx1M5Zgy6-3da0r1QTra6iL_0WGUWMw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACWgwAbvbh4yyu_MbtVojFq-yFkM-sO%2BrmHmPvA7ZHGvgmcMcw%40mail.gmail.com.

[Avi Drissman]

I'll take this.

[dan...@chromium.org]

On Tue, Feb 15, 2022 at 1:59 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, 15 Feb 2022 at 10:54, 'Avi Drissman' via cxx <c...@chromium.org> wrote:

Where do we go from here?

- Do we make a CL switching uses in our tree to absl::?

Yes.

- Do we add a presubmit (sigh) or a Tricium plugin to alert folks?

Yes to the presubmit.

 

Whatever happened to the stdlib hardening switch?

Mostly lack of anyone working on it :)

It got blocked on resistance upstream to letting us define our own assertion function, so it's enabled in tests and not in prod. And we're not expanding use of it as a result.

I recall something about some openness to revisit this, but yes there's no one working on it.

[Avi Drissman]

CL is https://crrev.com/c/3465476.

If we're not going to see any movement any time soon on this, do we mark a bunch of library functions as banned and add them to the presubmit? 

[dan...@chromium.org]

On Tue, Feb 15, 2022 at 2:50 PM Avi Drissman <a...@google.com> wrote:

CL is https://crrev.com/c/3465476.

If we're not going to see any movement any time soon on this, do we mark a bunch of library functions as banned and add them to the presubmit? 

Yes, that would be an accurate representation of where we are at, and we should enforce it for consistency and security.

[David Benjamin]

Would this be worth carrying a patch to libcxx for, at least temporarily, if upstream is slow or resistant? We'd end up looking like more standard C++17, and our hardening would be more robust. E.g. other C++17 code will probably just write std::optional without thinking about it, or being under Chromium PRESUBMITs. (Both external third-party projects and "third-party" code that is also maintained by Google or Chromium folks, but as a separate project to share code elsewhere.) We'd also be able to harden things like std::vector::operator[], which we currently use throughout.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSEVfu9AhN%2B7P7KfyJiov-G_n%2Br4LTw_4vT%3DP8gfyYSog%40mail.gmail.com.

[dan...@chromium.org]

Forking libc++ would be a pretty serious undertaking and require a staffing commitment, I think. I also dream of removing implicit conversions. :)

I don't think the value proposition outweighs using absl for this.

[Daniel Cheng]

I don't currently have the bandwidth to poke at this quarter. If someone is interested, it's probably best to sync up with thakis@ and figure out how to move forward. Otherwise, "maybe" I'll look at this in the future.

That being said, I would also be perfectly happy with "don't use std:: and only use our own collections/algorithms" :)

Daniel

[Daniel Cheng]

Just to be clear, by "moving forward", the ultimate goal would be to have libc++ be hardened similarly.

[Roland McGrath]

Today's upstream libc++ implementation of std::optional::value does reliably abort.  Its operator* and operator-> only check based on the `_LIBCPP_DEBUG` setting.  AFAICT the Chromium build enables `_LIBCPP_DEBUG` under `is_debug || dcheck_always_on` except for the Windows component build because of some bug.  That said, I don't think upstream libc++ makes any guarantees about this behavior beyond what the standard says.  (I think basically any place the standard guarantees an exception will be thrown, then libc++ without exceptions will guarantee you an abort.  The standard has guaranteed exception behavior for value() but UB for operator* / operator->.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrumqKJXe7rmYD%3DWSGn_kMtkceed25Bw%3DBxaUZsgd8S0g%40mail.gmail.com.

[David Benjamin]

I was thinking less of a capital-F Fork, in the sense of taking responsibility for all changes going forward, and more a patch that we rebase. We have some third_party directories with patches today. Though, yeah, that is still much more expensive than just having it upstream and may not be worth it. Getting something upstream would definitely be better. (I'm a little disappointed this isn't just an obvious security/safety improvement to upstream libc++. :-/)

In the long run, I don't think banning and otherwise ignoring std:: containers is good for our hardening efforts. It leaves an increasing amount of low-level parsing code uncovered by hardening. We end up sharing code between Chromium and other projects, particularly around networking. Our TLS stack is outside of Chromium, in BoringSSL. Our QUIC code and bits of HTTP/3 are in QUICHE. WebRTC is a standalone library. In the future, we'll be moving our certificate verifier to BoringSSL, and I expect more and more HTTP protocol code will happen in QUICHE, not //net. All of those will typically use standard stuff and may have constraints of their own. (E.g. BoringSSL cannot currently depend on absl, while we can depend on parts of the STL.)

[Avi Drissman]

To what extent can we rely on libcpp's existing hooks? The two examples I have std::clamp which has:

clamp(const _Tp& __v, const _Tp& __lo, const _Tp& __hi, _Compare __comp)
{
    _LIBCPP_ASSERT(!__comp(__hi, __lo), "Bad bounds passed to std::clamp");
    return __comp(__v, __lo) ? __lo : __comp(__hi, __v) ? __hi : __v;
}

and std::optional which has:

    constexpr value_type const& value() const&
    {
        if (!this->has_value())
            __throw_bad_optional_access();
        return this->__get();
    }

[...]

void __throw_bad_optional_access() {
#ifndef _LIBCPP_NO_EXCEPTIONS
        throw bad_optional_access();
#else
        _VSTD::abort();
#endif
}

How close to usable is libcpp if we make _LIBCPP_ASSERT be a CHECK?

I share David's concern. If you follow the link to std::optional above, you'll find lots of references to third_party uses of it. At some point, hiding in our cave and only using absl is going to fail us.

And if we've paid the price to be single-vendor re compiler, at least let's get some of the rewards by being able to rely on the hardening in it.

Avi

[Grace Cham]

Hi,

I learnt about this thread when presenting on the migration from libchrome base::Optional (and absl::optional) to std::optional in CrOS.

We had the same concerns then and talked to the CrOS tool chain team about it, see thread.

The discussion ended with that being something upstream libc++ will also like to see and they will work on decoupling hardening asserts from debugging asserts (null-dereference of optional and others being examples of hardening asserts).

Unfortunately the work got delayed and, as a temporary measure to unblock the migration, we patched CrOS libc++ to crash when dereferencing a nullopt (fyi, CL: crrev.com/c/3380901), similar to David's suggestion above.

+cjdb@ who helped us then. Chris, would you mind rerouting this to someone working on libc++ now for latest progress on the decoupling asserts work? Thanks!

Grace

[Christopher Di Bella]

+Eric Fiselier would you mind advising on the current libc++ situation please?

--

Kind regards,

Christopher Di Bella (he/him/his)

[dan...@chromium.org]

There's nothing too spooky going on but please take care about linking to internal group threads on public mailing lists.

[Hans Wennborg]

I think the decoupling part happened recently: https://reviews.llvm.org/D121478

We (chrome toolchain team) are interested in working on this. One prerequisite to make use of that patch is to bring our libc++ version up to date (crbug.com/1273285), which is also on our todo list.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKCW6pviWkyOh1VDwZTBKsofii9T7fTgYqFm%2BuFqWUWsHGQqaA%40mail.gmail.com.