Move UBSan warnings from a blacklist to the source

Move UBSan warnings from a blacklist to the source

This is done to make UBSan testing more convenient in integration with projects using WebRTC

Some blacklist entries were obsolete so don't need a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491
Review-Url: https://codereview.webrtc.org/3009123002
Cr-Commit-Position: refs/heads/master@{#19682}
Committed: https://chromium.googlesource.com/external/webrtc/+/8ad0e589e7ec1bbb126d118b551c7bda7a9de278

[oprypin_webrtc, 2017-09-04 19:15:00]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Also fix one of the warnings

BUG=
==========

to

==========
Move UBSan warnings from a blacklist to the source

Also fix one of the warnings

BUG=webrtc:5486
==========

[oprypin_webrtc, 2017-09-04 19:15:22]

The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 19:15:31]

Dry run: 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, 2017-09-04 19:27:00]

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

[commit-bot: I haz the power, 2017-09-04 19:27:01]

Dry run: Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/29374)

[oprypin_webrtc, 2017-09-04 19:36:12]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Also fix one of the warnings

BUG=webrtc:5486
==========

to

==========
Move UBSan warnings from a blacklist to the source

Also fix two of the warnings

BUG=webrtc:5486, webrtc:5491, webrtc:5513
==========

[oprypin_webrtc, 2017-09-04 20:25:13]

Patchset #1 (id:1) has been deleted

[oprypin_webrtc, 2017-09-04 20:26:23]

The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 20:26:30]

Dry run: CQ is trying da patch.

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

[oprypin_webrtc, 2017-09-04 20:29:09]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Also fix two of the warnings

BUG=webrtc:5486, webrtc:5491, webrtc:5513
==========

to

==========
Move UBSan warnings from a blacklist to the source

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:5486, webrtc:5491, webrtc:5513
==========

[oprypin_webrtc, 2017-09-04 20:36:39]

Patchset #3 (id:60001) has been deleted

[oprypin_webrtc, 2017-09-04 20:37:25]

The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 20:37:30]

Dry run: 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, 2017-09-04 21:11:01]

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

[commit-bot: I haz the power, 2017-09-04 21:11:02]

Dry run: This issue passed the CQ dry run.

[oprypin_webrtc, 2017-09-04 21:16:50]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:5486, webrtc:5491, webrtc:5513
==========

to

==========
Move UBSan warnings from a blacklist to the source

Remove obsolete blacklist entries without a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:5486, webrtc:5491, webrtc:8189
==========

[oprypin_webrtc, 2017-09-04 21:16:51]

oprypin@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, kwiberg@webrtc.org

[oprypin_webrtc, 2017-09-04 21:17:20]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Remove obsolete blacklist entries without a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:5486, webrtc:5491, webrtc:8189
==========

to

==========
Move UBSan warnings from a blacklist to the source

Remove obsolete blacklist entries without a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491
==========

[oprypin_webrtc, 2017-09-04 21:27:46]

Patchset #4 (id:100001) has been deleted

[oprypin_webrtc, 2017-09-04 21:30:44]

The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-04 21:30:51]

Dry run: CQ is trying da patch.

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

[oprypin_webrtc, 2017-09-04 21:42:38]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

Remove obsolete blacklist entries without a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491
==========

to

==========
Move UBSan warnings from a blacklist to the source

This is done to make UBSan testing more convenient in integration with projects
using WebRTC

Some blacklist entries were obsolete so don't need a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491
==========

[oprypin_webrtc, 2017-09-04 21:42:53]

oprypin@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[oprypin_webrtc, 2017-09-04 21:49:45]

Patchset #4 (id:120001) has been deleted

[kwiberg-webrtc, 2017-09-04 23:09:51]

Excellent! Much better than the centralized blacklist.

If you want to go one step further, defining tiny inline functions for just the
offending operations and slapping the no-sanitize attribute on them instead is
even more precise. See e.g.
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...

No-sanitizing the whole functions like you're doing here looks perfectly
acceptable, though, so your call.

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
File tools_webrtc/ubsan/blacklist.txt (right):

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:3: # Please think twice before you add or
remove these rules.
Should we have an explicit warning against putting WebRTC's own files here? That
stuff you just removed...

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:6: # adding WebRTC-specific blacklist entries.
Do we need our own copy now?

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.cc
File webrtc/rtc_base/random.cc (right):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:43: const auto low_i64 = static_cast<int64_t>(low);
This can be

  const int64_t low_i64{low};

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:45: low_i64);
Both of these static_casts can be proven to always preserve values, right? But
we don't expect the compiler to be able to prove it? In that case, they should
probably be dchecked_casts.

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/sanitizer.h
File webrtc/rtc_base/sanitizer.h (right):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/sanitize...
webrtc/rtc_base/sanitizer.h:14: #include <stddef.h>  // for size_t
This sort of comment would be useful, except that they so easily get out of date
that you can't trust them---which makes them useless. Better to just eliminate
the noise.

[kjellander_webrtc, 2017-09-05 04:56:04]

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/common_audio/sign...
File webrtc/common_audio/signal_processing/signal_processing_unittest.cc (left):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/common_audio/sign...
webrtc/common_audio/signal_processing/signal_processing_unittest.cc:44:
EXPECT_EQ(-2147483645, WEBRTC_SPL_MUL(a, b));
Is this intentional?

[oprypin_webrtc, 2017-09-05 07:27:34]

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
File tools_webrtc/ubsan/blacklist.txt (right):

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:3: # Please think twice before you add or
remove these rules.
On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> Should we have an explicit warning against putting WebRTC's own files here?
That
> stuff you just removed...

Yeah I guess. Reworded this.

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:6: # adding WebRTC-specific blacklist entries.
On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> Do we need our own copy now?

Yes. Not only do they have Chromium-specific function blacklists, but also are
missing some third_party library exceptions compared to this.

https://chromium.googlesource.com/chromium/src/tools/+/master/ubsan/blacklist...

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/common_audio/sign...
File webrtc/common_audio/signal_processing/signal_processing_unittest.cc (left):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/common_audio/sign...
webrtc/common_audio/signal_processing/signal_processing_unittest.cc:44:
EXPECT_EQ(-2147483645, WEBRTC_SPL_MUL(a, b));
On 2017/09/05 04:56:04, kjellander_webrtc wrote:
> Is this intentional?

Yes, see patch set 4

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.cc
File webrtc/rtc_base/random.cc (right):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:43: const auto low_i64 = static_cast<int64_t>(low);
On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> This can be
> 
>   const int64_t low_i64{low};

I used the code you suggested in
https://bugs.chromium.org/p/webrtc/issues/detail?id=8189&desc=3#c7
I checked that with this change it's still the same byte-for-byte, so I agree.
https://godbolt.org/g/upEsVg
Changed.

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:45: low_i64);
On 2017/09/04 23:09:51, kwiberg-webrtc wrote:
> Both of these static_casts can be proven to always preserve values, right? But
> we don't expect the compiler to be able to prove it? In that case, they should
> probably be dchecked_casts.

high - low_i64 always fits in uint32_t:
(2**31-1) - (-2**31) == (2**32-1)

Minimal result of Rand+low_i64 is 0 + (-2**31) == (-2**31)
Maximal result of Rand+low_i64 is (2**32-1) + (-2**31) == (2**31-1)

So seems like truncations are impossible here.
dchecked_cast will work. Changed.

[oprypin_webrtc, 2017-09-05 07:40:57]

The CQ bit was checked by oprypin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-09-05 07:41:01]

Dry run: CQ is trying da patch.

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

[kwiberg-webrtc, 2017-09-05 08:37:31]

lgtm

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
File tools_webrtc/ubsan/blacklist.txt (right):

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:3: # Please think twice before you add or
remove these rules.
On 2017/09/05 07:27:34, oprypin_webrtc wrote:
> On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> > Should we have an explicit warning against putting WebRTC's own files here?
> That
> > stuff you just removed...
> 
> Yeah I guess. Reworded this.

Nice.

https://codereview.webrtc.org/3009123002/diff/140001/tools_webrtc/ubsan/black...
tools_webrtc/ubsan/blacklist.txt:6: # adding WebRTC-specific blacklist entries.
On 2017/09/05 07:27:34, oprypin_webrtc wrote:
> On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> > Do we need our own copy now?
> 
> Yes. Not only do they have Chromium-specific function blacklists, but also are
> missing some third_party library exceptions compared to this.
> 
>
https://chromium.googlesource.com/chromium/src/tools/+/master/ubsan/blacklist...

Acknowledged.

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.cc
File webrtc/rtc_base/random.cc (right):

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:43: const auto low_i64 = static_cast<int64_t>(low);
On 2017/09/05 07:27:34, oprypin_webrtc wrote:
> On 2017/09/04 23:09:50, kwiberg-webrtc wrote:
> > This can be
> > 
> >   const int64_t low_i64{low};
> 
> I used the code you suggested in
> https://bugs.chromium.org/p/webrtc/issues/detail?id=8189&desc=3#c7
> I checked that with this change it's still the same byte-for-byte, so I agree.
> https://godbolt.org/g/upEsVg
> Changed.

Excellent. That matches my results from when I wrote that bug comment.

https://codereview.webrtc.org/3009123002/diff/140001/webrtc/rtc_base/random.c...
webrtc/rtc_base/random.cc:45: low_i64);
On 2017/09/05 07:27:34, oprypin_webrtc wrote:
> On 2017/09/04 23:09:51, kwiberg-webrtc wrote:
> > Both of these static_casts can be proven to always preserve values, right?
But
> > we don't expect the compiler to be able to prove it? In that case, they
should
> > probably be dchecked_casts.
> 
> high - low_i64 always fits in uint32_t:
> (2**31-1) - (-2**31) == (2**32-1)
> 
> Minimal result of Rand+low_i64 is 0 + (-2**31) == (-2**31)
> Maximal result of Rand+low_i64 is (2**32-1) + (-2**31) == (2**31-1)
> 
> So seems like truncations are impossible here.
> dchecked_cast will work. Changed.

Thanks for double-checking the math!

[oprypin_webrtc, 2017-09-05 08:54:31]

The CQ bit was unchecked by oprypin@webrtc.org

[oprypin_webrtc, 2017-09-05 08:54:35]

The CQ bit was checked by oprypin@webrtc.org

[commit-bot: I haz the power, 2017-09-05 08:54:43]

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, 2017-09-05 09:23:18]

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

[commit-bot: I haz the power, 2017-09-05 09:23:20]

Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/27519)

[oprypin_webrtc, 2017-09-05 09:51:49]

The CQ bit was checked by oprypin@webrtc.org

[commit-bot: I haz the power, 2017-09-05 09:51:59]

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, 2017-09-05 10:00:38]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1504605109161830,
"parent_rev": "a0c893f324da5c9f0773f8990a29cd24643864e4", "commit_rev":
"8ad0e589e7ec1bbb126d118b551c7bda7a9de278"}

[commit-bot: I haz the power, 2017-09-05 10:00:44]

Description was changed from

==========
Move UBSan warnings from a blacklist to the source

This is done to make UBSan testing more convenient in integration with projects
using WebRTC

Some blacklist entries were obsolete so don't need a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491
==========

to

==========
Move UBSan warnings from a blacklist to the source

This is done to make UBSan testing more convenient in integration with projects
using WebRTC

Some blacklist entries were obsolete so don't need a replacement

Also fix one of the warnings (thanks, kwiberg@)

BUG=webrtc:8189, webrtc:5486, webrtc:5491

Review-Url: https://codereview.webrtc.org/3009123002
Cr-Commit-Position: refs/heads/master@{#19682}
Committed:
https://chromium.googlesource.com/external/webrtc/+/8ad0e589e7ec1bbb126d118b5...
==========

[commit-bot: I haz the power, 2017-09-05 10:00:45]

Committed patchset #5 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/8ad0e589e7ec1bbb126d118b5...

[hlundin-webrtc, 2017-09-05 10:20:20]

RS LGTM