[vlc-devel] [PATCH] Fix for SAP mishandling of duplicates via different addresses

[Alexander Clouter]

Rémi Denis-Courmont <remi at remlab.net> wrote:
> 
> On Wed, 19 Oct 2011 14:21:56 +0100, Alexander Clouter <alex at digriz.org.uk>
> wrote:
>
>> We broadcast a number of IPTV channels at my organisation and want the
>> channels available to internal and nearby external users. As a result
>> we announce these channels via SAP on *both* 239.255.255.255
>> (SAP_V4_LOCAL_ADDRESS) and 239.195.255.255 (SAP_V4_ORG_ADDRESS)...
> 
> I really don't see the point, when you could use the largest scope for
> everyone.
> 
We have some streams that must stay private and thus should only be 
broadcasted on 239.255.255.255, the rest goes out on 239.195.255.255 
(the group addresses used for the streams are completely different too).  
We have a bunch of hardware set-top boxes (Exterity iDapters) that by 
default only tune into 239.255.255.255; if we were to enable them to 
tune into 239.195.255.255 too they see announcements from the whole 
.ac.uk[1] multicast community.

>> The function modules/services_discovery/sap.c::ParseSAP() has an 
>> IsSameSession() check that overlooks the possibility that the same 
>> session could arrive via different SAP listener sockets and as a 
>> result rewrites the entire SAP playlist once every 30 seconds
> 
> If the two announces are actually identical and VLC should match them and
> not rewrite anything. This is not what your patch does.
> 
> Or they are different and IsSameSession() should not match them, 
> irrelevant of the multicast group they were received from. Your patch 
> does not do that either.
>
They are identical, byte for byte, I just send them out over both 
239.255.255.255 and 239.195.255.255.

>> (we announce at a period of five seconds and your 'trust' cycle of 
>> six leads to 30 seconds).
> 
> This sounds more like a bug in the time/trust logic, that I never really
> understood.
>
Looking at things more closely, it looks like it's the "Check for items 
that need deletion" bit that gets upset back in the main run loop.  The 
duplicates mangle the calculation of average period; as the duplicates 
arrive ~1ms apart and then the cycle repeats at 5000ms intervals.

>> Once rewritten, any channel being viewed is terminated.
> 
> This is a known bug in the playlist code. Feel free to fix it.
>
That has a DeKok ring to it...

Besides, that particular bug does not directly bother me.

>> This repeats every thirty seconds, getting annoying, real quickly. :)
>> 
>> The solution is to additionally track the SAP listener socket that 
>> the packet arrived on, and only go through the 'trust' codepath if it 
>> comes in on the same socket; ignoring duplicates (packets that match 
>> the conditions of IsSameSession) arriving via different sockets.
>> 
>> I am pretty sure this is a safe fix as nothing in RFC2974 (section 5, 
>> 'Session Modification') implies that broadcasting the same SDP 
>> payload on different SAP addresses is incorrect; hopefully I have not 
>> missed any clauses ;)
> 
> Sure, but your patch seems to contradict that by actively fighting this.
> 
This is because I mis-diagnosed the underlying problem :)

The patch fixes the problem by preventing the value of i_last/i_period 
being tinkered with by the duplicate SAP inside the for loop lurking at 
ParseSAP()->...IsSameSession().

Cheers

[1] http://www.ja.net/documents/development/network-engineering/Multicast/MulticastAddressPolicyV3.pdf

-- 
Alexander Clouter
.sigmonster says: What I tell you three times is true.
                  		-- Lewis Carroll