Re: [reSIProcate] resiprocate parsing masks SDP errors

[Scott Godin <sgodin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx> 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="">

_______________________________________________
resiprocate-devel mailing list
resiprocate-devel@xxxxxxxxxxxxxxx
https://list.resiprocate.org/mailman/listinfo/resiprocate-devel