Fix some signed overflow errors causing undefined behavior (in theory).

Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491
Committed: https://crrev.com/d802b5b7c3f6b5e667bc7bf296de76010d4531e3
Cr-Commit-Position: refs/heads/master@{#11832}

[terelius, 2016-02-29 13:22:07]

Description was changed from

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=5491
==========

to

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491
==========

[terelius, 2016-02-29 13:22:07]

terelius@webrtc.org changed reviewers:
+ kjellander@webrtc.org, perkj@webrtc.org

[terelius, 2016-02-29 13:23:45]

Please take a look at this CL which fixes the signed overflow  errors detected
by UBSan.

[kjellander_webrtc, 2016-02-29 14:22:41]

Please address win compile errors

[kjellander_webrtc, 2016-02-29 14:23:12]

On 2016/02/29 14:22:41, kjellander (webrtc) wrote:
> Please address win compile errors

(modules_unittests failures were caused by
https://chromium.googlesource.com/external/webrtc/+/944744b25c76810e576516d2f...
which is now reverted)

[terelius, 2016-02-29 15:31:13]

On 2016/02/29 14:23:12, kjellander (webrtc) wrote:
> On 2016/02/29 14:22:41, kjellander (webrtc) wrote:
> > Please address win compile errors
> 
> (modules_unittests failures were caused by
>
https://chromium.googlesource.com/external/webrtc/+/944744b25c76810e576516d2f...
> which is now reverted)

Done.

The problem was that MVS warns when you negate an unsigned number.
Note that with the current setup, UBsan tests fail if you negate a
signed number and MVS fails to compile if you negate an unsigned
number.

[kjellander_webrtc, 2016-02-29 15:32:15]

lgtm but Per kan probably spot errors better than I in this code.

[perkj_webrtc, 2016-03-01 10:18:27]

perkj@webrtc.org changed reviewers:
+ torbjorng@webrtc.org

[perkj_webrtc, 2016-03-01 10:18:28]

Torbjörn, can you do this review?

[terelius, 2016-03-01 10:49:25]

On 2016/03/01 10:18:28, perkj_webrtc wrote:
> Torbjörn, can you do this review?

I believe I still need owner approval for base/ from you, Per.

Torbjörn, feel free to come by if you want to discuss the changes.

[torbjorng (webrtc), 2016-03-01 13:26:43]

lgtm

https://codereview.webrtc.org/1744183002/diff/20001/webrtc/base/ipaddress.cc
File webrtc/base/ipaddress.cc (right):

https://codereview.webrtc.org/1744183002/diff/20001/webrtc/base/ipaddress.cc#...
webrtc/base/ipaddress.cc:415: word_to_count &= ~word_to_count + 1;  // Isolate
lowest set bit.
Surely valid code, but perhaps mention (in the comment above) for what purpose
negation is done like this?  Else, a cleanup sweep may change this to plain "-".

[perkj_webrtc, 2016-03-01 13:47:48]

On 2016/03/01 13:26:43, torbjorng (webrtc) wrote:
> lgtm
> 
> https://codereview.webrtc.org/1744183002/diff/20001/webrtc/base/ipaddress.cc
> File webrtc/base/ipaddress.cc (right):
> 
>
https://codereview.webrtc.org/1744183002/diff/20001/webrtc/base/ipaddress.cc#...
> webrtc/base/ipaddress.cc:415: word_to_count &= ~word_to_count + 1;  // Isolate
> lowest set bit.
> Surely valid code, but perhaps mention (in the comment above) for what purpose
> negation is done like this?  Else, a cleanup sweep may change this to plain
"-".

rs lgtm

[terelius, 2016-03-01 17:00:03]

The CQ bit was checked by terelius@webrtc.org

[terelius, 2016-03-01 17:00:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
torbjorng@webrtc.org, kjellander@webrtc.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1744183002/#ps40001 (title:
"Format and comment")

[commit-bot: I haz the power, 2016-03-01 17:00:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1744183002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1744183002/40001

[commit-bot: I haz the power, 2016-03-01 17:02:55]

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

[commit-bot: I haz the power, 2016-03-01 17:02:56]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[terelius, 2016-03-01 18:14:45]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-03-01 18:14:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1744183002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1744183002/40001

[commit-bot: I haz the power, 2016-03-01 19:07:37]

Description was changed from

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491
==========

to

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491
==========

[commit-bot: I haz the power, 2016-03-01 19:07:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-01 19:07:46]

Description was changed from

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491
==========

to

==========
Fix some signed overflow errors causing undefined behavior (in theory).

BUG=webrtc:5491

Committed: https://crrev.com/d802b5b7c3f6b5e667bc7bf296de76010d4531e3
Cr-Commit-Position: refs/heads/master@{#11832}
==========

[commit-bot: I haz the power, 2016-03-01 19:07:47]

Patchset 3 (id:??) landed as
https://crrev.com/d802b5b7c3f6b5e667bc7bf296de76010d4531e3
Cr-Commit-Position: refs/heads/master@{#11832}

[kjellander_webrtc, 2016-03-01 21:04:53]

In order to benefit from these fixes, you'll need to also re-enable the disabled
tests in https://codereview.webrtc.org/1727233005 that were disabled because of
these errors. 
Can you take care of that in a follow-up CL? 
It's possible we can now revert that whole CL, but I'm not sure. Give it a try
locally before sending it to the CQ at least (and only the linux_ubsan bot
should be enough to run).