rtc::Optional: Tell sanitizers that unset values aren't OK to access

rtc::Optional: Tell sanitizers that unset values aren't OK to access

This is a sample use of the other three sanitizer functions introduced
in https://codereview.webrtc.org/2293893002/.

BUG=chromium:617124
Committed: https://crrev.com/2b11fd2e4bcbd5240ee04110323309f1fa2d3251
Cr-Commit-Position: refs/heads/master@{#14157}

[kwiberg-webrtc, 2016-09-03 13:48:05]

Patchset #1 (id:1) has been deleted

[kwiberg-webrtc, 2016-09-03 13:48:17]

Patchset #1 (id:20001) has been deleted

[kwiberg-webrtc, 2016-09-03 13:48:26]

Patchset #1 (id:40001) has been deleted

[kwiberg-webrtc, 2016-09-03 13:48:37]

Patchset #1 (id:60001) has been deleted

[kwiberg-webrtc, 2016-09-03 14:48:39]

kwiberg@webrtc.org changed reviewers:
+ ossu@webrtc.org, tommi@webrtc.org

[tommi, 2016-09-03 15:00:51]

lgtm

https://codereview.webrtc.org/2289383002/diff/80001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/80001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:219: } else {
nit: else not necessary

[kwiberg-webrtc, 2016-09-03 16:05:36]

New patch set uploaded.

https://codereview.webrtc.org/2289383002/diff/80001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/80001/webrtc/base/optional.h#ne...
webrtc/base/optional.h:219: } else {
On 2016/09/03 15:00:50, tommi (webrtc) wrote:
> nit: else not necessary 

Done.

[ossu, 2016-09-05 08:27:21]

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc
File webrtc/base/optional.cc (left):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc#...
webrtc/base/optional.cc:12: 
This all is just rietveld going crazy trying to find a source file for this
change, right?

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:29: void* FunctionThatDoesNothingImpl(void*);
I'm a bit cautious about this one. Since we can't hide it properly, (i.e. put it
in an anonymous namespace in the .cc file), there's a chance it will actually
clash if we want to do this for another class.
Wouldn't it be better if you stuffed this inside Optional, as a private (static)
member function? That or shoving it into some helper header, I guess.

[kwiberg-webrtc, 2016-09-05 09:11:30]

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc
File webrtc/base/optional.cc (left):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc#...
webrtc/base/optional.cc:12: 
On 2016/09/05 08:27:21, ossu wrote:
> This all is just rietveld going crazy trying to find a source file for this
> change, right?

Yes. (Specifically, I think it's git.)

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:29: void* FunctionThatDoesNothingImpl(void*);
On 2016/09/05 08:27:21, ossu wrote:
> I'm a bit cautious about this one. Since we can't hide it properly, (i.e. put
it
> in an anonymous namespace in the .cc file), there's a chance it will actually
> clash if we want to do this for another class.
> Wouldn't it be better if you stuffed this inside Optional, as a private
(static)
> member function? That or shoving it into some helper header, I guess.

No, I can't put it in rtc::Optional, because that's a template, and that doesn't
mesh well with this function's need to be non-inline.

Would it be OK if I instead changed the namespace to "optional_internal" or
something?

[ossu, 2016-09-05 09:38:43]

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc
File webrtc/base/optional.cc (left):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.cc#...
webrtc/base/optional.cc:12: 
On 2016/09/05 09:11:30, kwiberg-webrtc wrote:
> On 2016/09/05 08:27:21, ossu wrote:
> > This all is just rietveld going crazy trying to find a source file for this
> > change, right?
> 
> Yes. (Specifically, I think it's git.)

Acknowledged.

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:217: return has_value_ ?
*internal::FunctionThatDoesNothing(&value_)
If this is a problem for us here, how do we prevent it from being a problem in
the rest of the code?

I.e. if my function somewhere does if(opt) { return *opt; } or the (almost)
exact same thing as above: return opt ? *opt : "nothing!".
Is that safe or will it come back to bite us?

[kwiberg-webrtc, 2016-09-05 12:41:22]

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:217: return has_value_ ?
*internal::FunctionThatDoesNothing(&value_)
On 2016/09/05 09:38:43, ossu wrote:
> If this is a problem for us here, how do we prevent it from being a problem in
> the rest of the code?
> 
> I.e. if my function somewhere does if(opt) { return *opt; } or the (almost)
> exact same thing as above: return opt ? *opt : "nothing!".
> Is that safe or will it come back to bite us?

Yeah, it isn't safe in the general case, in that that there are probably other
constructs that will cause problems. The problems will only ever be observed in
ASan builds, though, so the potential damage is small.

In the slightly longer term, I'd like us to steal Chromium's base::Optional, and
that one has an inner class for the specific purpose of holding the value; if we
guard its accessors like this, it ought to be foolproof. Doing the same for our
current Optional class would take a fair bit of refactoring, which I'm not keen
on doing; it would be like washing your car when you're just about to ditch it
and steal a new one.

[ossu, 2016-09-08 08:49:03]

lgtm with a comment

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h
File webrtc/base/optional.h (right):

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:29: void* FunctionThatDoesNothingImpl(void*);
On 2016/09/05 09:11:30, kwiberg-webrtc wrote:
> On 2016/09/05 08:27:21, ossu wrote:
> > I'm a bit cautious about this one. Since we can't hide it properly, (i.e.
put
> it
> > in an anonymous namespace in the .cc file), there's a chance it will
actually
> > clash if we want to do this for another class.
> > Wouldn't it be better if you stuffed this inside Optional, as a private
> (static)
> > member function? That or shoving it into some helper header, I guess.
> 
> No, I can't put it in rtc::Optional, because that's a template, and that
doesn't
> mesh well with this function's need to be non-inline.
> 
> Would it be OK if I instead changed the namespace to "optional_internal" or
> something?

Ah, that's true. Yeah, optional_internal is fine by me!

https://codereview.webrtc.org/2289383002/diff/100001/webrtc/base/optional.h#n...
webrtc/base/optional.h:217: return has_value_ ?
*internal::FunctionThatDoesNothing(&value_)
On 2016/09/05 12:41:21, kwiberg-webrtc wrote:
> On 2016/09/05 09:38:43, ossu wrote:
> > If this is a problem for us here, how do we prevent it from being a problem
in
> > the rest of the code?
> > 
> > I.e. if my function somewhere does if(opt) { return *opt; } or the (almost)
> > exact same thing as above: return opt ? *opt : "nothing!".
> > Is that safe or will it come back to bite us?
> 
> Yeah, it isn't safe in the general case, in that that there are probably other
> constructs that will cause problems. The problems will only ever be observed
in
> ASan builds, though, so the potential damage is small.
> 
> In the slightly longer term, I'd like us to steal Chromium's base::Optional,
and
> that one has an inner class for the specific purpose of holding the value; if
we
> guard its accessors like this, it ought to be foolproof. Doing the same for
our
> current Optional class would take a fair bit of refactoring, which I'm not
keen
> on doing; it would be like washing your car when you're just about to ditch it
> and steal a new one.

Hmm... alright, let's put this in for now and revisit once we re-import Optional
from Chromium.

[kwiberg-webrtc, 2016-09-08 12:51:16]

The CQ bit was checked by kwiberg@webrtc.org

[kwiberg-webrtc, 2016-09-08 12:51:17]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
ossu@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2289383002/#ps140001 (title:
"rebase")

[commit-bot: I haz the power, 2016-09-08 12:51:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-08 13:29:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 13:29:46]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[kwiberg-webrtc, 2016-09-09 09:06:00]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2016-09-09 09:06:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-09 10:35:27]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-09-09 10:35:38]

Description was changed from

==========
rtc::Optional: Tell sanitizers that unset values aren't OK to access

This is a sample use of the other three sanitizer functions introduced
in https://codereview.webrtc.org/2293893002/.

BUG=chromium:617124
==========

to

==========
rtc::Optional: Tell sanitizers that unset values aren't OK to access

This is a sample use of the other three sanitizer functions introduced
in https://codereview.webrtc.org/2293893002/.

BUG=chromium:617124

Committed: https://crrev.com/2b11fd2e4bcbd5240ee04110323309f1fa2d3251
Cr-Commit-Position: refs/heads/master@{#14157}
==========

[commit-bot: I haz the power, 2016-09-09 10:35:40]

Patchset 4 (id:??) landed as
https://crrev.com/2b11fd2e4bcbd5240ee04110323309f1fa2d3251
Cr-Commit-Position: refs/heads/master@{#14157}