"implied_port" is improperly handled

[carlreinke]

If a message includes the "implied_port" argument, the message parser finds the "port" portion of the "implied_port" argument key and uses its value as the port, which is always 0 or 1.

See BEP 5 for proper handling of the "implied_port" argument.

[ghazel]

Aha, thank you https://github.com/clostra/dcdn/blob/master/client.c#L222

[jech]

Fixed in 7166f21. Thanks.

[jech]

I've also added explicit support for implicit_port in 5853ed1. This is completely untested, @carlreinke, @ghazel, please be so kind as too proof-read me.

[cfpp2p]

Tested 5853ed1 with transmission. No problems. Thousands of implied_port, every last one Both port and implied_port.

            if(implied_port != 0) {
                if(port != 0) {
                    debugf("Both port and implied_port.\n");
                    /* But continue, that's what the spec says. */
                }
                switch(from->sa_family) {
                case AF_INET:
                    port = htons(((struct sockaddr_in*)from)->sin_port);
                    debugf("Announce_peer: AF_INET implied_port %d.\n", port);
                    break;
                case AF_INET6:
                    port = htons(((struct sockaddr_in6*)from)->sin6_port);
                    debugf("Announce_peer: AF_INET6 implied_port %d.\n", port);
                    break;
                default:
                    port = 0;
                    debugf("Announce_peer: unsupported sa_family! implied port set zero.\n");
                    break;
                }
            }

[cfpp2p]

OK, it seems to be about 25 per thousand implied_port that don't have Both port and implied_port.

[jech]

Tested 5853ed1 with transmission. No problems.

Thanks for the feedback.

Thousands of implied_port, every last one Both port and implied_port.

Do you recommend removing the debug statement?

…

-- Juliusz

[cfpp2p]

I think it might make sense to log the inverse. Does this make sense?

            if(implied_port != 0) {
                if(port == 0) {
                    debugf("implied_port alone.\n");
                }
                /* Continue either way, that's what the spec says. */

[jech]

I think it might make sense to log the inverse. Does this make sense?

No, I don't think so. IMHO, port and implied_port should be exclusive.

[cfpp2p]

OK, recommend removing the debug statement. It's not about debugging the DHT.

[cfpp2p]

@ghazel

Aha, thank you https://github.com/clostra/dcdn/blob/master/client.c#L222

0.24 implementation and DHT pollution.

https://github.com/cfpp2p/dht-trs-trs/tree/port-logging logged:

Forty-two percent of 118,152 messages contained optional implied_port.

Of those, 98 percent contain both
non zero port and flagged (non zero) implied_port.
Twenty-four percent of those,
derived (implied) port value differed from port value.
With 0.24
twenty-four percent would have announce peer port incorrect,
NOT based on derived port, but on port.

With 0.24
remaining two percent
messages containing optional implied_port,
those with port=0 (or missing) and implied_port flagged on,
would then announce peer port 1.

Of course this is assuming that port precedes implied_port in the message, which I didn't check.

[ghazel]

Bencoding has lexigraphically sorted keys, so implied_port should come first.

[jech]

Bencoding has lexigraphically sorted keys, so implied_port should come first.

And therefore implied_port was taking priority, since memmem stops at the first match. Exactly the wrong behaviour. I'm feeling pretty bad about this bug. I'm seriously considering switching to a proper bencoding parser, but I want dht.c to remain a single-file implementation with minimal dependencies.

[ghazel]

Sounds like a fun challenge. How many lines would be acceptable?