Prevent memory corruption by StreamId::Set

Prevent memory corruption by StreamId::Set

Use RTC_CHECK to crash if attempting to set an RSID whose name's length exceeds the maximum.

BUG=None

Review-Url: https://codereview.webrtc.org/2915913003
Cr-Commit-Position: refs/heads/master@{#18405}
Committed: https://chromium.googlesource.com/external/webrtc/+/4a3c9f60a39b82ef27bd4582c088fa5af5a24566

[eladalon, 2017-06-01 15:40:03]

Description was changed from

==========
Prevent memory corruption by StreamId::Set

1. Use RTC_CHECK to crash if attempting to set an RSID whose name's length
exceeds the maximum.
2. In case someone has turned RTC_CHECK off, memcpy no more than kMaxSize
characters.

BUG=None
==========

to

==========
Prevent memory corruption by StreamId::Set

1. Use RTC_CHECK to crash if attempting to set an RSID whose name's length
exceeds the maximum.
2. In case someone has turned RTC_CHECK off, memcpy no more than kMaxSize
characters.

BUG=None
==========

[eladalon, 2017-06-01 15:40:03]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, stefan@webrtc.org

[eladalon, 2017-06-01 15:40:10]

PTAL

[danilchap, 2017-06-01 15:42:44]

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc#newcode29
webrtc/common_types.cc:29: memcpy(value_, data, std::min(size, kMaxSize));
you shouldn't try to recover after failed CHECK:
CHECK will crash the program, i.e. this line will run only if size <= kMaxSize

[eladalon, 2017-06-01 15:48:43]

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc#newcode29
webrtc/common_types.cc:29: memcpy(value_, data, std::min(size, kMaxSize));
On 2017/06/01 15:42:44, danilchap wrote:
> you shouldn't try to recover after failed CHECK:
> CHECK will crash the program, i.e. this line will run only if size <= kMaxSize

As per the CL's description, I'm afraid someone might turn off RTC_CHECKs. Some
third-party apps might turn it off. No?

[eladalon, 2017-06-01 15:49:05]

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

[commit-bot: I haz the power, 2017-06-01 15:49:10]

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-06-01 15:49:11]

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

[commit-bot: I haz the power, 2017-06-01 15:49:12]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-webrtc-committers".
Note that this has nothing to do with OWNERS files.

[danilchap, 2017-06-01 15:54:56]

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc#newcode29
webrtc/common_types.cc:29: memcpy(value_, data, std::min(size, kMaxSize));
On 2017/06/01 15:48:43, eladalon wrote:
> On 2017/06/01 15:42:44, danilchap wrote:
> > you shouldn't try to recover after failed CHECK:
> > CHECK will crash the program, i.e. this line will run only if size <=
kMaxSize
> 
> As per the CL's description, I'm afraid someone might turn off RTC_CHECKs.
Some
> third-party apps might turn it off. No?

DCHECKS can be turned off, CHECKS - can't. 
quote from base/checks.h: 
"RTC_CHECK always evaluates its argument, so it's OK for x to have side effects"

[eladalon, 2017-06-01 15:59:48]

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc
File webrtc/common_types.cc (right):

https://codereview.webrtc.org/2915913003/diff/1/webrtc/common_types.cc#newcode29
webrtc/common_types.cc:29: memcpy(value_, data, std::min(size, kMaxSize));
On 2017/06/01 15:54:56, danilchap wrote:
> On 2017/06/01 15:48:43, eladalon wrote:
> > On 2017/06/01 15:42:44, danilchap wrote:
> > > you shouldn't try to recover after failed CHECK:
> > > CHECK will crash the program, i.e. this line will run only if size <=
> kMaxSize
> > 
> > As per the CL's description, I'm afraid someone might turn off RTC_CHECKs.
> Some
> > third-party apps might turn it off. No?
> 
> DCHECKS can be turned off, CHECKS - can't. 
> quote from base/checks.h: 
> "RTC_CHECK always evaluates its argument, so it's OK for x to have side
effects"

Very well; undone.

[eladalon, 2017-06-01 16:00:08]

Description was changed from

==========
Prevent memory corruption by StreamId::Set

1. Use RTC_CHECK to crash if attempting to set an RSID whose name's length
exceeds the maximum.
2. In case someone has turned RTC_CHECK off, memcpy no more than kMaxSize
characters.

BUG=None
==========

to

==========
Prevent memory corruption by StreamId::Set

Use RTC_CHECK to crash if attempting to set an RSID whose name's length exceeds
the maximum.

BUG=None
==========

[eladalon, 2017-06-01 16:00:28]

Changed the CL description, too.

[danilchap, 2017-06-01 16:01:10]

lgtm

[eladalon, 2017-06-01 16:08:14]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-01 16:08:17]

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-06-01 16:12:55]

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

[commit-bot: I haz the power, 2017-06-01 16:12:56]

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

[eladalon, 2017-06-01 17:28:28]

stefan, could you please accept?

[holmer, 2017-06-02 07:27:30]

holmer@google.com changed reviewers:
+ holmer@google.com

[holmer, 2017-06-02 07:27:31]

lgtm

[eladalon, 2017-06-02 07:38:23]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-02 07:38: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, 2017-06-02 07:41:56]

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

[commit-bot: I haz the power, 2017-06-02 07:41:57]

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

[stefan-webrtc, 2017-06-02 08:43:34]

lgtm

[eladalon, 2017-06-02 10:35:04]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-02 10:35:10]

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-06-02 10:37:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496399704079940,
"parent_rev": "5522021b4522cef49d5b5d6fd3351baadfa0d898", "commit_rev":
"4a3c9f60a39b82ef27bd4582c088fa5af5a24566"}

[commit-bot: I haz the power, 2017-06-02 10:37:51]

Description was changed from

==========
Prevent memory corruption by StreamId::Set

Use RTC_CHECK to crash if attempting to set an RSID whose name's length exceeds
the maximum.

BUG=None
==========

to

==========
Prevent memory corruption by StreamId::Set

Use RTC_CHECK to crash if attempting to set an RSID whose name's length exceeds
the maximum.

BUG=None

Review-Url: https://codereview.webrtc.org/2915913003
Cr-Commit-Position: refs/heads/master@{#18405}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4a3c9f60a39b82ef27bd4582c...
==========

[commit-bot: I haz the power, 2017-06-02 10:37:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/4a3c9f60a39b82ef27bd4582c...