[reSIProcate] resiprocate parsing masks SDP errors

[Scott Godin]

I agree with your thinking here.  I think either logging it like you are or
offering a new setting / flag to stop (throw) on insufficiently defined
codecs in the m line would make sense.

Scott

On Fri, Sep 26, 2014 at 12:59 PM, John Gregg <jgregg at aylus.com> wrote:

>
> I have some test cases in which a device sends my resiprocate code some
> malformed SDP (in particular, I've been focusing on dynamic payload types
> in the 'm' line that don't match up with any "a=rtpmap" lines, but the
> exact nature of the protocol violation is not important here.) I see that
> in SdpContents.cxx, SdpContents::Session::Medium::codecs(), we wrap the
> actual call to parse() in a try/catch, and in the event of an exception, we
> just silently don't include the offending payload type in the rtpmap.
> Likewise, if we see a payload in the 'm' line that isn't also in an
> 'a=rtpmap" line and also isn't a static codec, we silently ignore that as
> well.
>
> I understand about "Be conservative in what you send, liberal in what you
> accept," but sometimes silently letting the other guy get away with what
> are definitely violations of protocol can make things worse for everyone.
> You might limp along doing something suboptimal for a long time with no one
> ever realizing it, when it might have been better to explicitly fail the
> call with a 4XX.
>
> Either way, it seems that the application developer ought to have
> discretion to make this call, but as the code is written, there is no way
> for the stack to communicate up to the app that this has happened. All my
> code knows about are the codecs that survived and were valid. It never
> knows about the ones that failed the parse.
>
> I'm not even sure what I'd like to happen ideally here. You certainly
> don't want to unconditionally abort parsing upon finding an error (so
> throwing an exception would be the wrong idea). Ideally, you'd want somehow
> to set a checkable status for the app to see if he wanted to, saying that
> there was a parsing error, and with a text field for more detail. Or
> something like that.
>
> At the very least, in my own code I have added the following InfoLog near
> the bottom of SdpContents::Session::Medium::codecs():
>
> if (ri != staticCodecs.end())
> {
> ...
> }
> else
> {
>     InfoLog(<< "SdpContents::Session::Medium::codecs invalid SDP: payload
> type " << *i << " is neither in an rtpmap nor is it a static codec");
> }
>
> Any ideas? Am I missing something?
>
> Thanks,
>
> -John Gregg
>
> FYI: here is a snippet to illustrate what I'm talking about (the 96 should
> be a 99, or the 99s should be 96s):
>
> m=video 7002 RTP/AVP 97 96
> c=IN IP4 10.0.23.14
> a=rtpmap:97 H264/90000
> a=fmtp:97 profile-level-id=42e028; packetization-mode=1; max-br=452;
> max-mbps=11880
> a=rtpmap:99 H263-2000/90000
> a=fmtp:99 profile=0;level=45
>
>
>
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20140929/5b81aff6/attachment.htm>