1305813 - Ensure RID isn't empty when constructing RTP header

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Check to make sure that the RID isn't empty before constructing the header.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: rid_add_empty_check.patch

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=847f0075acac

Comment 3

[Nils Ohlmeier [:drno]]

Does this bug still apply after the 57 merger?

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

I will check.

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This bug should have been closed with the 49 merge. The code in question was also removed in the 57 merge though, so it is valid issue again.

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1305813 - do not send empty StreamId;

Review commit: https://reviewboard.mozilla.org/r/151620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151620/

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

https://reviewboard.mozilla.org/r/151620/#review156900

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:329
(Diff revision 1)
>            current_rotation != kVideoRotation_0)
>          rtp_header->SetExtension<VideoOrientation>(current_rotation);
>        last_rotation_ = current_rotation;
>      }
>      if (rid) {
> -      rtp_header->SetExtensionWithLength<StreamId>(strlen(rid)-1, rid);
> +      size_t len = strlen(rid);

I'm wondering if we should protect against this actually in RTPSender::SetRID() instead or additionally?

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Nils Ohlmeier [:drno] from comment #7)
> I'm wondering if we should protect against this actually in
> RTPSender::SetRID() instead or additionally?

I am for checking additionally, and having an empty string act the same way as a nullptr.

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151620/diff/1-2/

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151620/diff/2-3/

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

https://reviewboard.mozilla.org/r/151620/#review156910

::: media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:328
(Diff revision 3)
>        if (frame_type == kVideoFrameKey || current_rotation != last_rotation_ ||
>            current_rotation != kVideoRotation_0)
>          rtp_header->SetExtension<VideoOrientation>(current_rotation);
>        last_rotation_ = current_rotation;
>      }
>      if (rid) {

This is now always going to be true.

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151620/diff/3-4/

Comment 13

[Nils Ohlmeier [:drno]]

Comment on attachment 8880504 [details]
Bug 1305813 - do not send empty StreamId;

https://reviewboard.mozilla.org/r/151620/#review156932

LGTM

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/895701cff94e
do not send empty StreamId;r=drno

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/895701cff94e