Created attachment 268668 [details] [review]
Proposed fix with a test.

Created attachment 268669 [details] [review]
Patch from git

Review of attachment 268669 [details] [review]:

Looks like a decent idea to me if there's a real problem (I'm assuming this is based on testing with an actual broken server). The downside is everyone using even more memory based on messages that are really cheap for malicious senders to send... Scary but I guess that's just the name of the game with upnp.

There's one area where I'm not sure if this works 100%: the "AL" (alternative locations) header users. I never see that header and I don't even know where it's specced... but we seem to support it.

::: libgssdp/gssdp-resource-browser.c
@@ +675,3 @@
         if (!usn)
                 return; /* No USN specified */
+        location = soup_message_headers_get_one (headers, "Location");

We don't currently bail out if Location is not present, but I believe we should. Maybe that should be in a separate patch though...

In any case you should reuse this header later in the function, where the same header is looked up again.

@@ +697,3 @@
 
+        /* If location does not match, expect that we missed bye bye packet */
+        if (resource && strcmp(resource->location, location) != 0) {

With the current implementation, both locations can be null here. Maybe we really should bail out on missing Location header early...

::: tests/gtest/test-regression.c
@@ +380,3 @@
                g_test_add_func ("/bugs/gnome/673150", test_bgo673150);
                g_test_add_func ("/bugs/gnome/682099", test_bgo682099);
+               g_test_add_func ("/missed_bye_bye", test_missed_bye_bye);

I don't personally mind the function names, but I think it's a good practice to keep the bug number somewhere and test path seems like a good place: "/bugs/gnome/724030"

>location

You are right.

>"AL"

I never saw AL and I don't understand what it is used for. Should I compare also AL?

> /missed_bye_bye

I did not have bug at the time the patch was written. I will fix this. But what is the right place for this test? test-regression or test-functional?

I will prepare another patch to fix the issue when I have replies to my questions.

I once had looked up where that AL stuff comes from and there was a reason for it but I've forgotten what and where and why.

I took a look at the SSDP ietf draft on a hunch and that's where AL (and missing location) comes from. This phrase is in pretty much every request, including the notifys:

> SHOULD include the service's location expressed through the
> Location and/or AL header.

So according to the ietf draft, it's fine to not have a location at all if you have something in AL... The UPnP spec on the other hand does not even mention AL and explicitly states that location header is required.

I really doubt anyone is using AL with GSSDP and the UPnP spec is certainly the important one here but ...  maybe you could make sure the patch copes with no location headers set (basically, don't mind about AL and use g_strcmp0() in the comparison)? That way functionality stays same just in case someone does use it.

(In reply to comment #4)
> > /missed_bye_bye
> 
> I did not have bug at the time the patch was written. I will fix this. But
> what is the right place for this test? test-regression or test-functional?

At least I don't care about that. As long as we run it I'm happy.

Created attachment 269113 [details] [review]
Patch v2

New patch addressing the comments
Changes:
* compare location + AL, not only location. Parse locations only once.
* discard ssdp:alive packets with no location + AL
* rename test to match bug number

Any chance to get this patch reviewed / merged?

I believe it is useful in general. 
For us it fixed discovery issues that are not possible to reproduce.

Pushed with minor style fixes