Prevent ever getting events based on own SSDP messages #684

Closed
vjrantal opened this Issue Mar 30, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@vjrantal
Member

vjrantal commented Mar 30, 2016

This PR #680 fixes the typical case where we want to avoid getting events from ourself when USN changes. However, in the code review, it was discussed that there might be edge cases where the implemented check isn't enough and one might get a peer unavailability change based on a byebye message originating from the peer itself (this isn't critical since the upper layers needs to handle this case anyways).

This issue is about implementing a bullet-proof logic to detect own SSDP messages.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Mar 30, 2016

Member

Specifically what we were thinking is that in the case of SSDP we get a location header. Even if we change our USN (the unique ID) the location header stays the same unless we call stop advertising. So an easy fix could be to just check if we get a notification that contains an unrecognized USN (read: old) but with our current location. Then we can just automatically filter it out.

Member

yaronyg commented Mar 30, 2016

Specifically what we were thinking is that in the case of SSDP we get a location header. Even if we change our USN (the unique ID) the location header stays the same unless we call stop advertising. So an easy fix could be to just check if we get a notification that contains an unrecognized USN (read: old) but with our current location. Then we can just automatically filter it out.

@yaronyg yaronyg added the 0 - Icebox label Mar 30, 2016

@yaronyg yaronyg added this to the V1 milestone Aug 3, 2016

@yaronyg yaronyg added 1 - Backlog and removed 0 - Icebox labels Aug 4, 2016

@yaronyg yaronyg added Node bug labels Sep 26, 2016

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Sep 26, 2016

Member

@chapko I believe you fixed this in thaliproject/Thali_CordovaPlugin#1103. If you agree then please close this issue.

Member

yaronyg commented Sep 26, 2016

@chapko I believe you fixed this in thaliproject/Thali_CordovaPlugin#1103. If you agree then please close this issue.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
Member

yaronyg commented Oct 6, 2016

@chapko

This comment has been minimized.

Show comment
Hide comment
@chapko

chapko Oct 6, 2016

Member

Yes it is fixed, but I wouldn't call it bullet-proof. It just stores several recently used peer identifiers and ignores all incoming SSDP messages containing these ids. However it stores only limited number of own identifiers (I set it to 10 in thaliConfig) so if you manage to stop and start advertising 10 times before you get your own notification from the 1st stop, then it will be detected as if it is another peer. It is unlikely to happen in the real world scenario and hardly can be abused, so I'm just closing this issue.

Member

chapko commented Oct 6, 2016

Yes it is fixed, but I wouldn't call it bullet-proof. It just stores several recently used peer identifiers and ignores all incoming SSDP messages containing these ids. However it stores only limited number of own identifiers (I set it to 10 in thaliConfig) so if you manage to stop and start advertising 10 times before you get your own notification from the 1st stop, then it will be detected as if it is another peer. It is unlikely to happen in the real world scenario and hardly can be abused, so I'm just closing this issue.

@chapko chapko closed this Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment