Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805023002:
Add read support of RtpStreamId/RepairedRtpStreamId header extensions. (Closed)
|
Created:
3 years, 8 months ago by danilchap Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, yujie_mao (webrtc), zhuangzesen_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman, pthatcher1, erikvarga, perkj_webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd read support of RtpStreamId/RepairedRtpStreamId header extensions.
BUG=webrtc:7433
Review-Url: https://codereview.webrtc.org/2805023002
Cr-Commit-Position: refs/heads/master@{#17759}
Committed: https://chromium.googlesource.com/external/webrtc/+/ef8d773d26bc4e9c39b78cd86318b2c9c07dc939
Patch Set 1 : . #
Total comments: 9
Patch Set 2 : repair -> repaired and other renamings #Patch Set 3 : +fuzzer #Patch Set 4 : fix msan warning #Patch Set 5 : +rtp_header_fuzzer #
Total comments: 6
Patch Set 6 : Use dedicated class for StreamId #Patch Set 7 : Rebase #
Total comments: 13
Patch Set 8 : More feedback #Patch Set 9 : Restore RtpStreamId::Parse function with std::string as an output parameter #Patch Set 10 : restore std::string in the fuzzer #Patch Set 11 : RtpStreamId::Parse return false on empty rsid #
Total comments: 6
Patch Set 12 : Added ParseMalformedRsid test #Patch Set 13 : Add data accessors and operator!= to StreamId #Patch Set 14 : Rebase #
Total comments: 4
Patch Set 15 : +Comments #
Total comments: 5
Patch Set 16 : +one more comment #Messages
Total messages: 55 (28 generated)
Patchset #1 (id:1) has been deleted
danilchap@webrtc.org changed reviewers: + nisse@webrtc.org, stefan@webrtc.org
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h#new... webrtc/common_types.h:725: char rid[16]; The SDP level uses "rid=", but the header extensions are not actually called "RIDs". I think you'd be better off with "rtp_stream_id" and "repaired_rtp_stream_id" here. If that's too long, perhaps "stream_id and "rsid" and "repaired_rsid". https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h#new... webrtc/common_types.h:727: char repair_rid[16]; The header extension can be more than 16 chars, even though the WebRTC API only allows WebRTC clients to be up to 16 chars. They will usually be 1-2 chars, I'm guessing. Why don't we just using a std::string here? https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:80: kRtpExtensionRepairRtpStreamId, It's a "Repaired Stream", not a "Repair Stream". Those two things mean the opposite. One is a reference to the thing being prepared (repaired) and the other is the thing that's doing the repairing (repair). What we have in the spec is the former. https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:118: // extension. I don't understand this TODO. Only the WebRTC API has the "full 16", no the RTP layer. The RTP layer isn't bounded. And what does "when webrtc will set the extension" mean?
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h#new... webrtc/common_types.h:725: char rid[16]; On 2017/04/06 21:17:34, pthatcher1 wrote: > The SDP level uses "rid=", but the header extensions are not actually called > "RIDs". I think you'd be better off with "rtp_stream_id" and > "repaired_rtp_stream_id" here. If that's too long, perhaps "stream_id and > "rsid" and "repaired_rsid". Since structure type already have 'rtp' in it, I'll go for stream_id/repaired_stream_id. https://codereview.webrtc.org/2805023002/diff/20001/webrtc/common_types.h#new... webrtc/common_types.h:727: char repair_rid[16]; On 2017/04/06 21:17:34, pthatcher1 wrote: > The header extension can be more than 16 chars, even though the WebRTC API only > allows WebRTC clients to be up to 16 chars. They will usually be 1-2 chars, > I'm guessing. Why don't we just using a std::string here? std::string will turn this and all dependent structures (RTPHeader, WebRtcRTPHeader) from POD. Which will break in some random places since this structures reset or copied with memset/memcpy sometimes. long-term would prefer to use std::string. https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:80: kRtpExtensionRepairRtpStreamId, On 2017/04/06 21:17:34, pthatcher1 wrote: > It's a "Repaired Stream", not a "Repair Stream". Those two things mean the > opposite. One is a reference to the thing being prepared (repaired) and the > other is the thing that's doing the repairing (repair). What we have in the > spec is the former. Done. also adjusted sample names in test to stress it. https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h:118: // extension. On 2017/04/06 21:17:34, pthatcher1 wrote: > I don't understand this TODO. Only the WebRTC API has the "full 16", no the RTP > layer. The RTP layer isn't bounded. > > And what does "when webrtc will set the extension" mean? Rewritten TODO from note to self to (hopefully) more readable note.
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:80: kRtpExtensionRepairRtpStreamId, On 2017/04/07 07:07:49, danilchap wrote: > On 2017/04/06 21:17:34, pthatcher1 wrote: > > It's a "Repaired Stream", not a "Repair Stream". Those two things mean the > > opposite. One is a reference to the thing being prepared (repaired) and the > > other is the thing that's doing the repairing (repair). What we have in the > > spec is the former. > > Done. > also adjusted sample names in test to stress it. Please also update cl title and description.
Description was changed from ========== Add read support of RtpStreamId/RepairRtpStreamId header extensions. BUG=webrtc:7433 ========== to ========== Add read support of RtpStreamId/RepairedRtpStreamId header extensions. BUG=webrtc:7433 ==========
On 2017/04/07 08:41:23, nisse-webrtc wrote: > https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... > File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): > > https://codereview.webrtc.org/2805023002/diff/20001/webrtc/modules/rtp_rtcp/i... > webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:80: > kRtpExtensionRepairRtpStreamId, > On 2017/04/07 07:07:49, danilchap wrote: > > On 2017/04/06 21:17:34, pthatcher1 wrote: > > > It's a "Repaired Stream", not a "Repair Stream". Those two things mean the > > > opposite. One is a reference to the thing being prepared (repaired) and the > > > other is the thing that's doing the repairing (repair). What we have in > the > > > spec is the former. > > > > Done. > > also adjusted sample names in test to stress it. > > Please also update cl title and description. Done, thank you.
lgtm
https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:727: char stream_id[16]; This is intended as an arbitrary octet string, which could include nul bytes? And the octet strings "foo", "foo\0", "foo\0\0" must be treated as distinct? Then it seems uint8_t would be more appropriate than char, and that would also signal that str* functions aren't appropriate. (And std::basic_string<uint8_t> instead of std::string where a string class is wanted). https://codereview.webrtc.org/2805023002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:450: RTC_DCHECK_LE(len + 1, sizeof(header->extension.stream_id)); len+1 here is a bit confusing, but I guess that's how |len| is used in the rest of this function?
https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:727: char stream_id[16]; On 2017/04/10 07:15:27, nisse-webrtc wrote: > This is intended as an arbitrary octet string, which could include nul bytes? > And the octet strings "foo", "foo\0", "foo\0\0" must be treated as distinct? > > Then it seems uint8_t would be more appropriate than char, and that would also > signal that str* functions aren't appropriate. (And std::basic_string<uint8_t> > instead of std::string where a string class is wanted). according to spec valid characters are [0-9A-Za-z], so "foo\0" is an invalid value. This check is not enforced during parsing for simplicity: It makes sense to validate the configured rsid, then invalid rsid would never match valid one. https://codereview.webrtc.org/2805023002/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:450: RTC_DCHECK_LE(len + 1, sizeof(header->extension.stream_id)); On 2017/04/10 07:15:27, nisse-webrtc wrote: > len+1 here is a bit confusing, but I guess that's how |len| is used in the rest > of this function? Agree, I'll make a small follow up cl to make len value in this function more readable.
https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:727: char stream_id[16]; On 2017/04/10 08:39:41, danilchap wrote: > On 2017/04/10 07:15:27, nisse-webrtc wrote: > > This is intended as an arbitrary octet string, which could include nul bytes? > > And the octet strings "foo", "foo\0", "foo\0\0" must be treated as distinct? > > > > Then it seems uint8_t would be more appropriate than char, and that would also > > signal that str* functions aren't appropriate. (And std::basic_string<uint8_t> > > instead of std::string where a string class is wanted). > > according to spec valid characters are [0-9A-Za-z], so "foo\0" is an invalid > value. > This check is not enforced during parsing for simplicity: > It makes sense to validate the configured rsid, then invalid rsid would never > match valid one. Then one alternative would be to always zero-pad the values to 16 bytes, and then drop the *_size members? That would make things look simpler here, and comparisons could be done with memcmp(stream_id, ..., sizeof(stream_id)). Might make things complex elsewhere, though, depending on how this is used. This option is similar to the arcane unix strncpy semantics. Or make it 17 bytes and always nul-terminated (and if needed to avoid false matches, reject received ids including nul characters). I don't have a strong opinion here, but I think it would be nice if the representation could be simplified.
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19388) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23523)
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/160001/webrtc/common_types.h#ne... webrtc/common_types.h:727: char stream_id[16]; On 2017/04/10 09:18:33, nisse-webrtc wrote: > On 2017/04/10 08:39:41, danilchap wrote: > > On 2017/04/10 07:15:27, nisse-webrtc wrote: > > > This is intended as an arbitrary octet string, which could include nul > bytes? > > > And the octet strings "foo", "foo\0", "foo\0\0" must be treated as distinct? > > > > > > Then it seems uint8_t would be more appropriate than char, and that would > also > > > signal that str* functions aren't appropriate. (And > std::basic_string<uint8_t> > > > instead of std::string where a string class is wanted). > > > > according to spec valid characters are [0-9A-Za-z], so "foo\0" is an invalid > > value. > > This check is not enforced during parsing for simplicity: > > It makes sense to validate the configured rsid, then invalid rsid would never > > match valid one. > > Then one alternative would be to always zero-pad the values to 16 bytes, and > then drop the *_size members? > > That would make things look simpler here, and comparisons could be done with > memcmp(stream_id, ..., sizeof(stream_id)). Might make things complex elsewhere, > though, depending on how this is used. This option is similar to the arcane unix > strncpy semantics. > > Or make it 17 bytes and always nul-terminated (and if needed to avoid false > matches, reject received ids including nul characters). > > I don't have a strong opinion here, but I think it would be nice if the > representation could be simplified. I've created a dedicated class StreamId and moved all complexity there. This way it become easy to remove size property and make rest of the code simpler.
Looks pretty good. I have a few comments, sorry for being picky. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc#n... webrtc/common_types.cc:27: RTC_DCHECK_LE(size, kMaxSize); I'd like an RTC_DCHECK(memchr(data, 0, size) == nullptr) here, to make sure conversion makes sense. Or if you don't want to repeat this as a DCHECK here and a production check in the parsing functions, you could let this method have a bool return value to indicate success. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.h#ne... webrtc/common_types.h:715: void Set(const void* data, size_t size); I would have expected const char* or const uint8_t* here. Now, there's a conversion between uint8_t (used by the parsing functions) and char (used for the supposedly ascii only |value_|). Which is going to be a bit ugly in one way or the other. But I'm not sure it's the right way to introduce void* in the interface. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:224: if (data.empty()) If we use nul-termination in StreamId, I think ids containing nul characters should be rejected here. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:237: if (data.empty()) And here too.
Don't be sorry for pointing out unclear bits, your comments make code more readable. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc#n... webrtc/common_types.cc:27: RTC_DCHECK_LE(size, kMaxSize); On 2017/04/10 12:25:00, nisse-webrtc wrote: > I'd like an RTC_DCHECK(memchr(data, 0, size) == nullptr) here, to make sure > conversion makes sense. > > Or if you don't want to repeat this as a DCHECK here and a production check in > the parsing functions, you could let this method have a bool return value to > indicate success. Prefer to find those value acceptible. Added comment instead. One of the reason - to make tests simpler and more natural: currently StreamId created there from const char[N] where N would be string length including terminating zero, but I still treat them as one-char shorter strings. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.h#ne... webrtc/common_types.h:715: void Set(const void* data, size_t size); On 2017/04/10 12:25:00, nisse-webrtc wrote: > I would have expected const char* or const uint8_t* here. Now, there's a > conversion between uint8_t (used by the parsing functions) and char (used for > the supposedly ascii only |value_|). Which is going to be a bit ugly in one way > or the other. But I'm not sure it's the right way to introduce void* in the > interface. changed to const char* and made the cast explicit. There still inconsistency between constructor and one-parameter Set function. But I'm not sure how to remove it without adding conversion to code outside StreamId. [tried overloading - it make it worse because blocks implicit conversion from char[N] parameter] https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:224: if (data.empty()) On 2017/04/10 12:25:00, nisse-webrtc wrote: > If we use nul-termination in StreamId, I think ids containing nul characters > should be rejected here. General rule suggest to be strict about spec when writing, but use best-effort when reading the data. If some implementation of webrtc would incorrectly write value "1\0", It still would be preferable to treat it as "1" rather than discard as invalid.
Patchset #9 (id:240001) has been deleted
https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc#n... webrtc/common_types.cc:27: RTC_DCHECK_LE(size, kMaxSize); On 2017/04/10 13:27:05, danilchap wrote: > On 2017/04/10 12:25:00, nisse-webrtc wrote: > > I'd like an RTC_DCHECK(memchr(data, 0, size) == nullptr) here, to make sure > > conversion makes sense. > > > > Or if you don't want to repeat this as a DCHECK here and a production check in > > the parsing functions, you could let this method have a bool return value to > > indicate success. > > Prefer to find those value acceptible. Added comment instead. > > One of the reason - to make tests simpler and more natural: currently StreamId > created there from const char[N] where N would be string length including > terminating zero, but I still treat them as one-char shorter strings. Is that that the ArrayView constructor? I think that's a bit counter-intuitive when it comes to string literals, I would have expected ArrayView<char>("foo") to construct an array of length 3. If that can't be fixed, could tests go via a std::string instead, to get the intended length? https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:224: if (data.empty()) On 2017/04/10 13:27:05, danilchap wrote: > On 2017/04/10 12:25:00, nisse-webrtc wrote: > > If we use nul-termination in StreamId, I think ids containing nul characters > > should be rejected here. > > General rule suggest to be strict about spec when writing, but use best-effort > when reading the data. > If some implementation of webrtc would incorrectly write value "1\0", It still > would be preferable to treat it as "1" rather than discard as invalid. "Be liberal in what you accept, and conservative in what you send" Let me quote a bit more of RFC 1122, Sec. 1.2.2, to give some context for this advice, which is often misunderstood (and which I don't think recommends acceptance of invalid data): Software should be written to deal with every conceivable error, no matter how unlikely; sooner or later a packet will come in with that particular combination of errors and attributes, and unless the software is prepared, chaos can ensue. [...] The second part of the principle is almost as important: software on other hosts may contain deficiencies that make it unwise to exploit legal but obscure protocol features. It is unwise to stray far from the obvious and simple, lest untoward effects result elsewhere. It's a bit difficult to see any security implications in this particular case, but I think it's generally pretty bad to let different pieces of the code use different definition of the length of the same piece of protocol data. E.g., this version will accept a stream id of a single nul-byte (length 1), but if that value is later interpreted as the empty string, that violates reasonable assumptions other parts of the code can make about the stream id, since by the spec it can never be non-empty.
https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/common_types.cc#n... webrtc/common_types.cc:27: RTC_DCHECK_LE(size, kMaxSize); On 2017/04/11 07:59:24, nisse-webrtc wrote: > On 2017/04/10 13:27:05, danilchap wrote: > > On 2017/04/10 12:25:00, nisse-webrtc wrote: > > > I'd like an RTC_DCHECK(memchr(data, 0, size) == nullptr) here, to make sure > > > conversion makes sense. > > > > > > Or if you don't want to repeat this as a DCHECK here and a production check > in > > > the parsing functions, you could let this method have a bool return value to > > > indicate success. > > > > Prefer to find those value acceptible. Added comment instead. > > > > One of the reason - to make tests simpler and more natural: currently StreamId > > created there from const char[N] where N would be string length including > > terminating zero, but I still treat them as one-char shorter strings. > > Is that that the ArrayView constructor? I think that's a bit counter-intuitive > when it comes to string literals, I would have expected ArrayView<char>("foo") > to construct an array of length 3. > std::string_view would make more sense for StreamId, but that is available only in c++17 > If that can't be fixed, could tests go via a std::string instead, to get the > intended length? When it is same complexity, I choose std::string in tests, but when testing parsing RTPHeader, then comparing StreamId looks cleaner https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:224: if (data.empty()) On 2017/04/11 07:59:24, nisse-webrtc wrote: > On 2017/04/10 13:27:05, danilchap wrote: > > On 2017/04/10 12:25:00, nisse-webrtc wrote: > > > If we use nul-termination in StreamId, I think ids containing nul characters > > > should be rejected here. > > > > General rule suggest to be strict about spec when writing, but use best-effort > > when reading the data. > > If some implementation of webrtc would incorrectly write value "1\0", It still > > would be preferable to treat it as "1" rather than discard as invalid. > > "Be liberal in what you accept, and > conservative in what you send" > > Let me quote a bit more of RFC 1122, Sec. 1.2.2, I guess that was the quote I meant. was interesting to read the details, but there was nothing about accepting or rejecting malformed data. Parser shouldn't crash, and both guessing data based on invalid one [my approach] and validating and rejecting invalid data [yours] follow the guidance. > It's a bit difficult to see any security implications in this particular case, > but I think it's generally pretty bad to let different pieces of the code use > different definition of the length of the same piece of protocol data. Parser is the only one that see unparsed size. Once parsed, all the rest code expected to use the result of the parser and shouldn't be able to see difference between "1" and "1\0". > E.g., this version will accept a stream id of a single nul-byte (length 1), but > if that value is later interpreted as the empty string, that violates reasonable > assumptions other parts of the code can make about the stream id, since by the > spec it can never be non-empty. Good point about empty string, since empty rsid match absent of the extension, it make sense to guarantee Parse return false in this case. For other cases - I have another reason to prefer current way: it keeps code simpler. https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:237: if (data.empty()) On 2017/04/10 12:25:00, nisse-webrtc wrote: > And here too. ensured RapairedRtpStreamId::Parse implementation is same as RtpStreamId::Parse
https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/200001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:224: if (data.empty()) On 2017/04/11 10:05:26, danilchap wrote: > Good point about empty string, since empty rsid match absent of the extension, > it make sense to guarantee Parse return false in this case. > > For other cases - I have another reason to prefer current way: it keeps code > simpler. Fair enough. I'd prefer stricter parsing, but I don't insist. https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:231: bool RtpStreamId::Parse(rtc::ArrayView<const uint8_t> data, std::string* rsid) { Are the Parse methods with a std::string result for tests only? I think it would be better to use the same Parse method in tests as in the non-test code. https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:450: header->extension.stream_id.Set(rtc::MakeArrayView(ptr, len + 1)); This (and kRtpExtensionRepairedRtpStreamId below) would also accept a length one stream id containing only a nul-byte, right? So for consistency, check if ptr[0] == 0, and ignore the extension in that case?
https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:231: bool RtpStreamId::Parse(rtc::ArrayView<const uint8_t> data, std::string* rsid) { On 2017/04/11 10:42:53, nisse-webrtc wrote: > Are the Parse methods with a std::string result for tests only? > > I think it would be better to use the same Parse method in tests as in the > non-test code. no, std::string is for users of the rtp::Packet, StreamId - for users of the RTPHeader. One alternative way - move StreamId parse to rtp_utility, but would prefer to keep two parsing versions closer to each other to be more sure they are same. Another alternative way - parse to StreamId, then convert it to a std::string, but that would involve extra memcpy. https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:450: header->extension.stream_id.Set(rtc::MakeArrayView(ptr, len + 1)); On 2017/04/11 10:42:53, nisse-webrtc wrote: > This (and kRtpExtensionRepairedRtpStreamId below) would also accept a length one > stream id containing only a nul-byte, right? > > So for consistency, check if ptr[0] == 0, and ignore the extension in that case? Yes, but that would produce a stream_id that is .empty() which is consistent with 'no extension was present' case. Added test.
lgtm! https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:231: bool RtpStreamId::Parse(rtc::ArrayView<const uint8_t> data, std::string* rsid) { On 2017/04/11 12:51:26, danilchap wrote: > On 2017/04/11 10:42:53, nisse-webrtc wrote: > > Are the Parse methods with a std::string result for tests only? > > > > I think it would be better to use the same Parse method in tests as in the > > non-test code. > > no, std::string is for users of the rtp::Packet, > StreamId - for users of the RTPHeader. > One alternative way - move StreamId parse to rtp_utility, but would prefer to > keep two parsing versions closer to each other to be more sure they are same. > Another alternative way - parse to StreamId, then convert it to a std::string, > but that would involve extra memcpy. Ok. https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right): https://codereview.webrtc.org/2805023002/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_utility.cc:450: header->extension.stream_id.Set(rtc::MakeArrayView(ptr, len + 1)); On 2017/04/11 12:51:26, danilchap wrote: > On 2017/04/11 10:42:53, nisse-webrtc wrote: > > This (and kRtpExtensionRepairedRtpStreamId below) would also accept a length > one > > stream id containing only a nul-byte, right? > > > > So for consistency, check if ptr[0] == 0, and ignore the extension in that > case? > > Yes, but that would produce a stream_id that is .empty() > which is consistent with 'no extension was present' case. > Added test. Ok.
Stefan, ptal, specially at the changes to /common_types.h
lgtm % nits https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.cc#n... webrtc/common_types.cc:34: static_assert(std::is_trivially_destructible<StreamId>::value, ""); Maybe add a message explaining why this must be true? https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.h#ne... webrtc/common_types.h:699: class StreamId { I think we need a comment on what the definition of a StreamId is. For instance, it seems like the first element of the StreamId can't be 0. Is it a string of length 16, essentially?
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.cc#n... webrtc/common_types.cc:34: static_assert(std::is_trivially_destructible<StreamId>::value, ""); On 2017/04/19 06:46:16, stefan-webrtc_OOO_Apr_18 wrote: > Maybe add a message explaining why this must be true? Done. https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/360001/webrtc/common_types.h#ne... webrtc/common_types.h:699: class StreamId { On 2017/04/19 06:46:16, stefan-webrtc_OOO_Apr_18 wrote: > I think we need a comment on what the definition of a StreamId is. For instance, > it seems like the first element of the StreamId can't be 0. Is it a string of > length 16, essentially? Done.
https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.cc#n... webrtc/common_types.cc:35: // and thus asume trivial destructibility. Spelling of "assume". https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h#ne... webrtc/common_types.h:700: // Unlike std::string, it can be copied with memcpy and cleared with memset. I'd suggest to describe it like "NUL-terminated string of at most 16 characters, empty if no stream id is set.". Except that it isn't always NUL-terminated. Aren't users likely to assume that the data() method returns a NUL-terminated string, usable with plain C functions? You could extend |value_| with one byte more, and set value[kMaxSize] = 0.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.cc File webrtc/common_types.cc (right): https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.cc#n... webrtc/common_types.cc:35: // and thus asume trivial destructibility. On 2017/04/19 08:30:08, nisse-webrtc wrote: > Spelling of "assume". thank you! https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h#ne... webrtc/common_types.h:700: // Unlike std::string, it can be copied with memcpy and cleared with memset. On 2017/04/19 08:30:08, nisse-webrtc wrote: > empty if no stream id is set.". Done > > I'd suggest to describe it like "NUL-terminated string of at most 16 characters". Except that it isn't always NUL-terminated. I do not want to give assumption about NUL-termination (using 'most of the time is NUL-terminated' as an implementation detail) > Aren't users likely to assume that > the data() method returns a NUL-terminated string, usable with plain C > functions? I hope users will not make this assumption, otherwise I would name it c_str()
https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h File webrtc/common_types.h (right): https://codereview.webrtc.org/2805023002/diff/380001/webrtc/common_types.h#ne... webrtc/common_types.h:700: // Unlike std::string, it can be copied with memcpy and cleared with memset. On 2017/04/19 08:45:22, danilchap wrote: > I hope users will not make this assumption, otherwise I would name it c_str() Ok.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, nisse@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2805023002/#ps400001 (title: "+one more comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/23431)
The CQ bit was checked by danilchap@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492594480861850,
"parent_rev": "6f27633f47d0192f7cb04852be0988013e6c19dd", "commit_rev":
"ef8d773d26bc4e9c39b78cd86318b2c9c07dc939"}
Message was sent while issue was closed.
Description was changed from ========== Add read support of RtpStreamId/RepairedRtpStreamId header extensions. BUG=webrtc:7433 ========== to ========== Add read support of RtpStreamId/RepairedRtpStreamId header extensions. BUG=webrtc:7433 Review-Url: https://codereview.webrtc.org/2805023002 Cr-Commit-Position: refs/heads/master@{#17759} Committed: https://chromium.googlesource.com/external/webrtc/+/ef8d773d26bc4e9c39b78cd86... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:400001) as https://chromium.googlesource.com/external/webrtc/+/ef8d773d26bc4e9c39b78cd86... |
