Fixing logic for using android_setsocknetwork() with bind().

Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

This CL also changes "NetworkBinderResults" to an enum class, and
renames it to "NetworkBinderResult".

BUG=webrtc:7026
Review-Url: https://codereview.webrtc.org/2646863005
Cr-Commit-Position: refs/heads/master@{#16597}
Committed: https://chromium.googlesource.com/external/webrtc/+/c874d1296a96bd234c17fb73c0327d69b7f7c5dd

[Taylor Brandstetter, 2017-01-20 22:08:09]

Description was changed from

==========
If Android network binder is available, use it instead of bind().

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

to

==========
If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

BUG=webrtc:7026
==========

[Taylor Brandstetter, 2017-01-20 22:08:39]

Description was changed from

==========
If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

BUG=webrtc:7026
==========

to

==========
Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

BUG=webrtc:7026
==========

[Taylor Brandstetter, 2017-01-20 22:09:18]

Description was changed from

==========
Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

BUG=webrtc:7026
==========

to

==========
Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

This CL also changes "NetworkBinderResults" to an enum class, and
renames it to "NetworkBinderResult".

BUG=webrtc:7026
==========

[pthatcher1, 2017-02-10 18:12:18]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2017-02-10 18:12:19]

Any way we can add a unit test?

https://codereview.webrtc.org/2646863005/diff/60001/webrtc/base/physicalsocke...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/2646863005/diff/60001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:215: return -1;
Should we return different errors based on the result?  In fact, should we
return a NetworkBindingResult from this method?

[Taylor Brandstetter, 2017-02-10 18:57:52]

A test would require configuring a physical device with specific network
interfaces and routes; I'm not aware of any other tests that do that, but if
there is a way I'll do it.

https://codereview.webrtc.org/2646863005/diff/60001/webrtc/base/physicalsocke...
File webrtc/base/physicalsocketserver.cc (right):

https://codereview.webrtc.org/2646863005/diff/60001/webrtc/base/physicalsocke...
webrtc/base/physicalsocketserver.cc:215: return -1;
On 2017/02/10 18:12:19, pthatcher1 wrote:
> Should we return different errors based on the result?  In fact, should we
> return a NetworkBindingResult from this method?

Bind either returns -1 or 0, and I wasn't planning to change that in this CL.
There would be no point in returning a NetworkBindError now since nothing would
use it.

[pthatcher1, 2017-02-10 22:01:00]

Could you pass in a FakeNetworkBinder and test some behavior based on what
result you return from the FakeNetworkBinder?

[Taylor Brandstetter, 2017-02-10 22:29:52]

On 2017/02/10 22:01:00, pthatcher1 wrote:
> Could you pass in a FakeNetworkBinder and test some behavior based on what
> result you return from the FakeNetworkBinder?

Good idea, that's much better than nothing. Done

[pthatcher1, 2017-02-10 22:41:46]

lgtm

[Taylor Brandstetter, 2017-02-10 22:48:02]

deadbeef@webrtc.org changed reviewers:
+ magjed@webrtc.org

[Taylor Brandstetter, 2017-02-10 22:48:03]

magjed@: PTAL at JNI code

[magjed_webrtc, 2017-02-13 16:01:49]

jni code lgtm

[Taylor Brandstetter, 2017-02-13 17:26:56]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-02-13 17:27:06]

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-02-13 17:41:02]

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

[commit-bot: I haz the power, 2017-02-13 17:41:04]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/21992)
  win_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/23149)

[Taylor Brandstetter, 2017-02-13 18:46:08]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-13 18:46:09]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2646863005/#ps100001 (title:
"Only use fake network binder in test that intends to use it.")

[commit-bot: I haz the power, 2017-02-13 18:46:16]

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-02-13 19:13:56]

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

[commit-bot: I haz the power, 2017-02-13 19:13:57]

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

[Taylor Brandstetter, 2017-02-13 21:44:14]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-13 21:44:15]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2646863005/#ps120001 (title:
"Don't treat failing to bind to loopback interface as an error.")

[commit-bot: I haz the power, 2017-02-13 21:44:18]

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-02-13 21:56:47]

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

[commit-bot: I haz the power, 2017-02-13 21:56:48]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/22000)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22726)

[Taylor Brandstetter, 2017-02-13 22:33:12]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-13 22:33:12]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2646863005/#ps140001 (title:
"Fixing tests for loopback IP change.")

[commit-bot: I haz the power, 2017-02-13 22:33:24]

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-02-13 23:41:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487025192074160,
"parent_rev": "06b7e5ce1f3430db60a9ee1d7a201b82798459c2", "commit_rev":
"c874d1296a96bd234c17fb73c0327d69b7f7c5dd"}

[commit-bot: I haz the power, 2017-02-13 23:42:03]

Description was changed from

==========
Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

This CL also changes "NetworkBinderResults" to an enum class, and
renames it to "NetworkBinderResult".

BUG=webrtc:7026
==========

to

==========
Fixing logic for using android_setsocknetwork() with bind().

If android_setsocknetwork() is available, and it fails, then bind()
should *not* be called, and an error should be returned.

If it succeeds, then bind should be called, but with an "any" address.

This is to prevent cases where sockets are sent with a source address
that doesn't match the network interface they're sent on. See bug below.

This CL also changes "NetworkBinderResults" to an enum class, and
renames it to "NetworkBinderResult".

BUG=webrtc:7026

Review-Url: https://codereview.webrtc.org/2646863005
Cr-Commit-Position: refs/heads/master@{#16597}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c874d1296a96bd234c17fb73c...
==========

[commit-bot: I haz the power, 2017-02-13 23:42:04]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/c874d1296a96bd234c17fb73c...