Intensify security checks for WTF::Optional.

Intensify security checks for WTF::Optional.

Use ASSERT_WITH_SECURITY_IMPLICATION for dereference checks, and store a pointer
internally so that bugs manifest as null dereference in release, rather than
use of uninitialized memory, similar to OwnPtr.

BUG=492743
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196359

[jbroman, 2015-06-02 14:13:20]

jbroman@chromium.org changed reviewers:
+ esprehn@chromium.org, pdr@chromium.org

[jbroman, 2015-06-02 14:13:21]

Like similar checks elsewhere in WTF (e.g. Vector), esprehn correctly pointed
out that these should be ASSERT_WITH_SECURITY_IMPLICATION.

[jbroman, 2015-06-02 14:30:14]

And this latest patchset changes access to use a pointer which is null until
initialization, with a RELEASE_ASSERT that it isn't done twice.

This makes the checks against accessing uninitialized memory stronger than those
required by OwnPtr (but should still be acceptably inexpensive).

[esprehn, 2015-06-02 22:13:08]

lgtm w/ fix.

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h
File Source/wtf/Optional.h (right):

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h#n...
Source/wtf/Optional.h:43: operator UnspecifiedBoolType() const { return m_ptr ?
&Optional::m_ptr : nullptr; }
This doesn't need the branch in it, you can just return m_ptr.

return m_ptr;

should be enough, you also don't need the Optional:: part.

[jbroman, 2015-06-02 23:40:17]

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h
File Source/wtf/Optional.h (right):

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h#n...
Source/wtf/Optional.h:43: operator UnspecifiedBoolType() const { return m_ptr ?
&Optional::m_ptr : nullptr; }
On 2015/06/02 22:13:08, esprehn wrote:
> This doesn't need the branch in it, you can just return m_ptr.
> 
> return m_ptr;
> 
> should be enough, you also don't need the Optional:: part.

This is the pattern used in OwnPtr::operator UnspecifiedBoolType(), which also
has an m_ptr member.

m_ptr isn't of type UnspecifiedBoolType, so returning that would not compile; I
would have to return something constructible from T*. Using "operator bool" is
the obvious choice, but that would make this implicitly convertible to all
integer types (see comment in OwnPtr.h).

The modern fix for the is to use "explicit operator bool", but our Windows
toolchain doesn't yet support that (see "explicit conversion operator" on
http://chromium-cpp.appspot.com/).

So this is the hack/pattern we currently use elsewhere in WTF, ugly though it
is.

(Aside: I suspect, without having checked, that after inlining, the compiler can
eliminate this branch. Not sure though.)

[esprehn, 2015-06-02 23:57:08]

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h
File Source/wtf/Optional.h (right):

https://codereview.chromium.org/1159623012/diff/20001/Source/wtf/Optional.h#n...
Source/wtf/Optional.h:43: operator UnspecifiedBoolType() const { return m_ptr ?
&Optional::m_ptr : nullptr; }
Right, just return a void* or something. Having a branch inside the boolean
operator is not needed and just makes it slower.

[esprehn, 2015-06-02 23:59:40]

Hmm I guess we already do this for WTF, that's kind of silly but okay. Land this
and I'll look into removing the branches later.

[jbroman, 2015-06-03 00:06:50]

Patchset #3 (id:40001) has been deleted

[jbroman, 2015-06-03 00:06:55]

Patchset #3 (id:60001) has been deleted

[jbroman, 2015-06-03 00:09:10]

On 2015/06/02 23:59:40, esprehn wrote:
> Hmm I guess we already do this for WTF, that's kind of silly but okay. Land
this
> and I'll look into removing the branches later.

Okay, landing as is.

[jbroman, 2015-06-03 00:09:11]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-06-03 00:10:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159623012/20001

[jbroman, 2015-06-03 00:10:31]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2015-06-03 00:11:06]

Actually, still need someone with sufficient OWNERS powers. pdr?

[pdr., 2015-06-03 00:15:43]

LGTM

[jbroman, 2015-06-03 00:15:53]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2015-06-03 00:17:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1159623012/20001

[commit-bot: I haz the power, 2015-06-03 00:20:44]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196359