std::optional

101 views
Skip to first unread message

Avi Drissman

unread,
Feb 15, 2022, 1:45:58 PMFeb 15
to cxx
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

unread,
Feb 15, 2022, 1:47:33 PMFeb 15
to Avi Drissman, cxx
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

unread,
Feb 15, 2022, 1:54:33 PMFeb 15
to Dana Jansens, cxx
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

unread,
Feb 15, 2022, 1:59:43 PMFeb 15
to Avi Drissman, Dana Jansens, cxx
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.

Avi Drissman

unread,
Feb 15, 2022, 2:01:38 PMFeb 15
to Daniel Cheng, Dana Jansens, cxx
I'll take this.

dan...@chromium.org

unread,
Feb 15, 2022, 2:48:20 PMFeb 15
to Daniel Cheng, Avi Drissman, cxx
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

unread,
Feb 15, 2022, 2:50:57 PMFeb 15
to Dana Jansens, Daniel Cheng, cxx
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

unread,
Feb 15, 2022, 2:53:21 PMFeb 15
to Avi Drissman, Daniel Cheng, cxx
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

unread,
Feb 23, 2022, 6:09:32 PMFeb 23
to dan...@chromium.org, Avi Drissman, Daniel Cheng, cxx
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.

dan...@chromium.org

unread,
Feb 23, 2022, 6:12:27 PMFeb 23
to David Benjamin, Avi Drissman, Daniel Cheng, cxx
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

unread,
Feb 23, 2022, 6:14:09 PMFeb 23
to dan...@chromium.org, David Benjamin, Avi Drissman, cxx
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

unread,
Feb 23, 2022, 6:14:30 PMFeb 23
to dan...@chromium.org, David Benjamin, Avi Drissman, cxx
Just to be clear, by "moving forward", the ultimate goal would be to have libc++ be hardened similarly.

Roland McGrath

unread,
Feb 23, 2022, 6:30:23 PMFeb 23
to Daniel Cheng, Dana Jansens, David Benjamin, Avi Drissman, cxx
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->.)

David Benjamin

unread,
Feb 23, 2022, 6:35:29 PMFeb 23
to Daniel Cheng, dan...@chromium.org, Avi Drissman, cxx
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

unread,
Feb 23, 2022, 6:58:37 PMFeb 23
to David Benjamin, Daniel Cheng, Dana Jansens, cxx
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

unread,
Mar 29, 2022, 3:55:24 AMMar 29
to cxx, Avi Drissman, dch...@chromium.org, danakj, cxx, David Benjamin, Christopher Di Bella
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

unread,
Mar 29, 2022, 1:52:25 PMMar 29
to Grace Cham, Eric Fiselier, cxx, Avi Drissman, dch...@chromium.org, danakj, David Benjamin
+Eric Fiselier would you mind advising on the current libc++ situation please?
--
Kind regards,

Christopher Di Bella (he/him/his)

dan...@chromium.org

unread,
Mar 29, 2022, 2:48:28 PMMar 29
to Christopher Di Bella, Grace Cham, Eric Fiselier, cxx, Avi Drissman, dch...@chromium.org, David Benjamin
There's nothing too spooky going on but please take care about linking to internal group threads on public mailing lists.

Hans Wennborg

unread,
Mar 31, 2022, 7:55:56 AMMar 31
to Christopher Di Bella, Grace Cham, Eric Fiselier, cxx, Avi Drissman, dch...@chromium.org, danakj, David Benjamin
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.

Reply all
Reply to author
Forward
0 new messages