Add NAT traversal #209

Closed
wants to merge 21 commits into
from

Conversation

@Ansa89

Ansa89 commented Oct 25, 2016

Requested in TokTok/c-toxcore#195


This change is Reviewable

@zetok

zetok requested changes Oct 25, 2016 edited

  • missing docs about new dep(s?)
@iphydf

Before I start to review:

  • Make miniupnp and natmpm optional, so if one can't be found, it's not used.
  • Install both libraries on Travis, so the code can be compiled and tested.

Ansa89 added some commits Oct 25, 2016

@zetok

zetok approved these changes Oct 25, 2016

@GrayHatter GrayHatter added this to the v0.0.4 milestone Oct 26, 2016

.travis.yml
@@ -35,6 +35,8 @@ addons:
- opam # For apidsl and Frama-C.
- portaudio19-dev # For av_test.
- texinfo # For libconfig.
+ - libminiupnpc-dev # For UPnP.
+ - libnatpmp-dev # For NAT-PMP.

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Can you sort these?

@iphydf

iphydf Oct 26, 2016

Member

Can you sort these?

cmake/FindMiniUPnP.cmake
+ mark_as_advanced(MINIUPNP_INCLUDE_DIR MINIUPNP_LIBRARY)
+ set(MINIUPNP_LIBRARIES ${MINIUPNP_LIBRARY})
+ set(MINIUPNP_INCLUDE_DIRS ${MINIUPNP_INCLUDE_DIR})
+endif()

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

These two "Find" modules seem like they could use some abstraction. They are almost identical copies, so you could make a common find function that you pass the library name and header file name.

@iphydf

iphydf Oct 26, 2016

Member

These two "Find" modules seem like they could use some abstraction. They are almost identical copies, so you could make a common find function that you pass the library name and header file name.

toxcore/Messenger.h
@@ -26,6 +26,7 @@
#ifndef MESSENGER_H
#define MESSENGER_H
+#include "tox.h"

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

This creates an upward dependency. Generally, the solution is duplication and casting (I don't really like this solution) or using int and dealing with integer constants in the code (I don't like this solution either). Perhaps we can come up with a better solution?

@iphydf

iphydf Oct 26, 2016

Member

This creates an upward dependency. Generally, the solution is duplication and casting (I don't really like this solution) or using int and dealing with integer constants in the code (I don't like this solution either). Perhaps we can come up with a better solution?

This comment has been minimized.

toxcore/TCP_server.c
#endif
+#include "util.h"
+#include "nat_traversal.h"

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Sort includes.

@iphydf

iphydf Oct 26, 2016

Member

Sort includes.

toxcore/TCP_server.c
@@ -1086,6 +1101,16 @@ TCP_Server *new_TCP_server(uint8_t ipv6_enabled, uint16_t num_sockets, const uin
#endif
+#ifdef HAVE_LIBMINIUPNPC
+ if ((traversal_type == TOX_TRAVERSAL_TYPE_UPNP) || (traversal_type == TOX_TRAVERSAL_TYPE_ALL))
+ upnp_map_port(NAT_TRAVERSAL_TCP,ntohs(ports[i]));

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Space after comma. Run other/astyle/format-source.

@iphydf

iphydf Oct 26, 2016

Member

Space after comma. Run other/astyle/format-source.

toxcore/nat_traversal.c
+
+ error = UPNP_GetValidIGD(devlist, &urls, &data, lanaddr, sizeof(lanaddr));
+ freeUPNPDevlist(devlist);
+ if (error) {

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Use switch.

@iphydf

iphydf Oct 26, 2016

Member

Use switch.

toxcore/nat_traversal.c
+ return;
+ }
+
+ if (error != 12) {

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Add comment about this 12. A reader doesn't necessarily know that 12 means success, and why.

@iphydf

iphydf Oct 26, 2016

Member

Add comment about this 12. A reader doesn't necessarily know that 12 means success, and why.

toxcore/nat_traversal.c
+/* Setup port forwarding using NAT-PMP */
+void natpmp_map_port(NAT_TRAVERSAL_PROTO proto, uint16_t port)
+{
+ LOGGER_DEBUG("Attempting to set up NAT-PMP port forwarding");

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

None of this logger stuff works. You need to pass a logger. One of the changes in c-toxcore is that we no longer use a global logger for all tox instances.

@iphydf

iphydf Oct 26, 2016

Member

None of this logger stuff works. You need to pass a logger. One of the changes in c-toxcore is that we no longer use a global logger for all tox instances.

toxcore/nat_traversal.h
+#if defined(HAVE_LIBMINIUPNPC) || defined(HAVE_LIBNATPMP)
+typedef enum NAT_TRAVERSAL_PROTO {
+
+ /* UDP */

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Not a very useful comment. Remove or elaborate.

@iphydf

iphydf Oct 26, 2016

Member

Not a very useful comment. Remove or elaborate.

toxcore/tox.c
#include "Messenger.h"
#include "group.h"
#include "logger.h"
#include "../toxencryptsave/defines.h"
+#include "tox.h"

This comment has been minimized.

@iphydf

iphydf Oct 26, 2016

Member

Why did this move?

@iphydf

iphydf Oct 26, 2016

Member

Why did this move?

@nurupo

This comment has been minimized.

Show comment
Hide comment
@nurupo

nurupo Oct 27, 2016

Member

@iphydf are we going to add upnp to the tox spec?

Member

nurupo commented Oct 27, 2016

@iphydf are we going to add upnp to the tox spec?

toxcore/TCP_server.c
#include "util.h"
#if !defined(_WIN32) && !defined(__WIN32__) && !defined (WIN32)
#include <sys/ioctl.h>
+#include <arpa/inet.h>

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

Sort.

@iphydf

iphydf Oct 27, 2016

Member

Sort.

toxcore/TCP_server.c
@@ -25,11 +25,14 @@
#endif
#include "TCP_server.h"
-
+#include "nat_traversal.h"

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

Add empty line after the first include.

@iphydf

iphydf Oct 27, 2016

Member

Add empty line after the first include.

toxcore/nat_traversal.c
+#endif
+
+#include "nat_traversal.h"
+#include "logger.h"

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

Sort.

@mannol I said this before on exactly this line. Github reviews simply got rid of my comment. Github--.

@iphydf

iphydf Oct 27, 2016

Member

Sort.

@mannol I said this before on exactly this line. Github reviews simply got rid of my comment. Github--.

toxcore/nat_traversal.c
+ bool local_logger = false;
+ struct UPNPDev *devlist = NULL;
+
+ if (log == NULL) {

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

log should not be NULL.

@iphydf

iphydf Oct 27, 2016

Member

log should not be NULL.

toxcore/nat_traversal.c
+ }
+
+ error = readnatpmpresponseorretry(&natpmp, &resp);
+ for ( ; error == NATPMP_TRYAGAIN ; error = readnatpmpresponseorretry(&natpmp, &resp) )

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

Again: run other/astyle/format-source. This code has several style violations.

@iphydf

iphydf Oct 27, 2016

Member

Again: run other/astyle/format-source. This code has several style violations.

toxcore/nat_traversal.h
+/**
+ * The protocol that will be used by the nat traversal.
+ */
+#if defined(HAVE_LIBMINIUPNPC) || defined(HAVE_LIBNATPMP)

This comment has been minimized.

@iphydf

iphydf Oct 27, 2016

Member

Conditional compilation is not really necessary here. Can you abstract the traversal stuff away and have just a single function instead of two? It's not nice that both TCP_server.c and network.c needs to know about the existence of these two libraries.

@iphydf

iphydf Oct 27, 2016

Member

Conditional compilation is not really necessary here. Can you abstract the traversal stuff away and have just a single function instead of two? It's not nice that both TCP_server.c and network.c needs to know about the existence of these two libraries.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Oct 27, 2016

Member

@Ansa89 thanks, I saw you've fixed the format-code script. I've also done that in the just-merged #204 PR. Can you rebase on master?

Member

iphydf commented Oct 27, 2016

@Ansa89 thanks, I saw you've fixed the format-code script. I've also done that in the just-merged #204 PR. Can you rebase on master?

Ansa89 added some commits Oct 27, 2016

toxcore/TCP_server.c
+ return new_TCP_server_nat(log, ipv6_enabled, num_sockets, ports, TOX_TRAVERSAL_TYPE_NONE, secret_key, onion);
+}
+
+TCP_Server *new_TCP_server_nat(Logger *log, uint8_t ipv6_enabled, uint16_t num_sockets, const uint16_t *ports,

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

File a bug, assign to yourself, and add a TODO(#NNN) with NNN being the bug number, to remove new_TCP_server and rename new_TCP_server_nat to new_TCP_server. Add the same TODO to the other locations where you have _nat variants.

@iphydf

iphydf Oct 28, 2016

Member

File a bug, assign to yourself, and add a TODO(#NNN) with NNN being the bug number, to remove new_TCP_server and rename new_TCP_server_nat to new_TCP_server. Add the same TODO to the other locations where you have _nat variants.

This comment has been minimized.

@@ -0,0 +1,198 @@
+/* nat_traversal.c -- Functions to traverse a NAT (UPnP, NAT-PMP).

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

I'm not a fan of putting the file name into the file. It's another place to update when the file is renamed.

@iphydf

iphydf Oct 28, 2016

Member

I'm not a fan of putting the file name into the file. It's another place to update when the file is renamed.

toxcore/nat_traversal.c
+#include <miniupnpc/miniupnpc.h>
+#include <miniupnpc/miniwget.h>
+#include <miniupnpc/upnpcommands.h>
+#include <miniupnpc/upnperrors.h>

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

Sort.

@iphydf

iphydf Oct 28, 2016

Member

Sort.

toxcore/nat_traversal.c
+ return;
+ }
+
+ if (proto == NAT_TRAVERSAL_UDP) {

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

switch like above.

@iphydf

iphydf Oct 28, 2016

Member

switch like above.

toxcore/nat_traversal.c
+ }
+
+ if (error) {
+ LOGGER_WARNING(log, "NAT-PMP port mapping failed (error = %d)", error);

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

strnatpmperr

@iphydf

iphydf Oct 28, 2016

Member

strnatpmperr

toxcore/nat_traversal.c
+ int error;
+ struct UPNPDev *devlist = NULL;
+
+ LOGGER_DEBUG(log, "Attempting to set up UPnP port forwarding");

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

Move to the top of the function.

@iphydf

iphydf Oct 28, 2016

Member

Move to the top of the function.

toxcore/nat_traversal.c
+#include <natpmp.h>
+#endif
+
+#include "nat_traversal.h"

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

Move to before the system includes.

@iphydf

iphydf Oct 28, 2016

Member

Move to before the system includes.

toxcore/nat_traversal.c
+ LOGGER_DEBUG(log, "Attempting to set up UPnP port forwarding");
+
+#if MINIUPNPC_API_VERSION < 14
+ devlist = upnpDiscover(1000, NULL, NULL, 0, 0, &error);

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

Move declaration of devlist to here. Duplicate it on the #else branch.

@iphydf

iphydf Oct 28, 2016

Member

Move declaration of devlist to here. Duplicate it on the #else branch.

toxcore/nat_traversal.c
+ break;
+
+ case 3:
+ LOGGER_WARNING(log, "UPnP device was found but not recoginzed as IGD.");

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

"recognised" (or "recognized")

@iphydf

iphydf Oct 28, 2016

Member

"recognised" (or "recognized")

toxcore/nat_traversal.c
+#endif
+
+ if (error) {
+ LOGGER_WARNING(log, "UPnP discovery failed (%s)", strupnperror(error));

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

FYI (and for other reviewers): strupnperror is specified to return NULL for unknown errors. That would be undefined behaviour. In reality, however, strupnperror returns non-NULL in all cases.

@iphydf

iphydf Oct 28, 2016

Member

FYI (and for other reviewers): strupnperror is specified to return NULL for unknown errors. That would be undefined behaviour. In reality, however, strupnperror returns non-NULL in all cases.

+ break;
+
+ case NAT_TRAVERSAL_TCP:
+ error = UPNP_AddPortMapping(urls.controlURL, data.first.servicetype, portstr, portstr, lanaddr, "Tox", "TCP", 0, "0");

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

This line is exactly equal to the one in the UDP branch, apart from the "TCP" string. You can factor out the common code and use some "nat_traversal_name" function or just assign some variable. Perhaps use NULL in the unknown proto case.

@iphydf

iphydf Oct 28, 2016

Member

This line is exactly equal to the one in the UDP branch, apart from the "TCP" string. You can factor out the common code and use some "nat_traversal_name" function or just assign some variable. Perhaps use NULL in the unknown proto case.

toxcore/nat_traversal.c
+ }
+
+ if (error) {
+ LOGGER_WARNING(log, "UPnP port mapping failed (%s)", strupnperror(error));

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

In the unknown proto case, this will show "UnknownError", because error will still be set to 1, but there is no such error code (https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/upnperrors.c). Is this desirable?

@iphydf

iphydf Oct 28, 2016

Member

In the unknown proto case, this will show "UnknownError", because error will still be set to 1, but there is no such error code (https://github.com/miniupnp/miniupnp/blob/master/miniupnpc/upnperrors.c). Is this desirable?

@@ -551,6 +585,10 @@ bool tox_options_get_udp_enabled(const struct Tox_Options *options);
void tox_options_set_udp_enabled(struct Tox_Options *options, bool udp_enabled);
+TOX_TRAVERSAL_TYPE tox_options_get_traversal_type(const struct Tox_Options *options);

This comment has been minimized.

@iphydf

iphydf Oct 28, 2016

Member

The declarations of these functions are generated by apidsl. There are some macros for creating the definitions as well. Can you add them? See the beginning of tox.c.

@iphydf

iphydf Oct 28, 2016

Member

The declarations of these functions are generated by apidsl. There are some macros for creating the definitions as well. Can you add them? See the beginning of tox.c.

@GrayHatter

This comment has been minimized.

Show comment
Hide comment
@GrayHatter

GrayHatter Oct 29, 2016

I'd like to review this before it gets merged, but I wont' have the time until the 31st. So if everyone else is okay with it, you can unassign me and merge.

GrayHatter commented Oct 29, 2016

I'd like to review this before it gets merged, but I wont' have the time until the 31st. So if everyone else is okay with it, you can unassign me and merge.

@iphydf

iphydf approved these changes Oct 30, 2016

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Oct 30, 2016

Member

@Ansa89 I'm switching everything over to Reviewable again. Github reviews is simply too broken.

Member

iphydf commented Oct 30, 2016

@Ansa89 I'm switching everything over to Reviewable again. Github reviews is simply too broken.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Oct 30, 2016

Member

Reviewed 3 of 19 files at r1, 1 of 10 files at r2, 1 of 14 files at r3, 2 of 2 files at r4, 7 of 14 files at r5.
Review status: 14 of 24 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.


INSTALL.md, line 560 at r6 (raw file):

###NAT traversal:

Some home-routers support UPnP and/or NAT-PMP to simplify port forwarding, hence toxcore can be compiled with support for those protocols.

Replace ", hence t" with ". T". It's not really something that follows from the other. Making it two sentences doesn't make this statement weaker.


toxcore/nat_traversal.c, line 203 at r6 (raw file):

    bool use_status = (status != NULL) ? true : false;

    if (use_status) {

Instead of use_status, you can just use status. If you insist on not using the implicit pointer to bool conversion, you can use status != NULL everywhere. Putting it into a variable moves the invariant check away from the code that relies on the invariant, making it a bit harder to reason about its correctness.


toxcore/nat_traversal.c, line 212 at r6 (raw file):

        if (use_status) {
            status->upnp = NAT_TRAVERSAL_ERR_UNKNOWN_TYPE;
            status->natpmp = NAT_TRAVERSAL_ERR_UNKNOWN_TYPE;

I can see why you did this, but it doesn't quite fit there. Unknown type is a global error condition and doesn't have anything to do with either upnp or natpmp. I don't know what's the best thing to do here. You could return false in this case, or just ignore it. If you insist on giving some diagnostic for invalid enum values, then the validity check for this type should be done during input validation (i.e. in tox.c). At this point in the code, we can then statically prove that this is dead code.

For the proxy type, we check it in tox.c, for savedata type, we just ignore invalid values. I'll leave it up to you what to do here - just providing some thoughts.


toxcore/nat_traversal.c, line 222 at r6 (raw file):

    if ((traversal_type == TOX_TRAVERSAL_TYPE_UPNP) || (traversal_type == TOX_TRAVERSAL_TYPE_ALL)) {
        if (use_status) {
            status->upnp = upnp_map_port(log, proto, port);

You could make these 4 lines: upnp_map_port(log, proto, port, status ? &status->upnp : NULL);. Same for natpmp.


toxcore/nat_traversal.c, line 251 at r6 (raw file):

const char *str_nat_traversal_error(NAT_TRAVERSAL_STATUS status)
{
    char *err = calloc(MAX_ERR_LENGTH + 1, sizeof(char));

This allocation leaks on every call. Since you're not doing any string formatting, you could just return the string literal. Replace all strcpy+breaks with return "message";.


toxcore/tox.api.h, line 363 at r6 (raw file):

 * Type of technology used to try to traverse a NAT.
 */
enum class TRAVERSAL_TYPE {

What do you think about making this a bitfield instead of an enum class? It currently is a bitfield by accident: UPNP | NATPMP == ALL.

Note that this is a question, don't change things before discussing.


Comments from Reviewable

Member

iphydf commented Oct 30, 2016

Reviewed 3 of 19 files at r1, 1 of 10 files at r2, 1 of 14 files at r3, 2 of 2 files at r4, 7 of 14 files at r5.
Review status: 14 of 24 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.


INSTALL.md, line 560 at r6 (raw file):

###NAT traversal:

Some home-routers support UPnP and/or NAT-PMP to simplify port forwarding, hence toxcore can be compiled with support for those protocols.

Replace ", hence t" with ". T". It's not really something that follows from the other. Making it two sentences doesn't make this statement weaker.


toxcore/nat_traversal.c, line 203 at r6 (raw file):

    bool use_status = (status != NULL) ? true : false;

    if (use_status) {

Instead of use_status, you can just use status. If you insist on not using the implicit pointer to bool conversion, you can use status != NULL everywhere. Putting it into a variable moves the invariant check away from the code that relies on the invariant, making it a bit harder to reason about its correctness.


toxcore/nat_traversal.c, line 212 at r6 (raw file):

        if (use_status) {
            status->upnp = NAT_TRAVERSAL_ERR_UNKNOWN_TYPE;
            status->natpmp = NAT_TRAVERSAL_ERR_UNKNOWN_TYPE;

I can see why you did this, but it doesn't quite fit there. Unknown type is a global error condition and doesn't have anything to do with either upnp or natpmp. I don't know what's the best thing to do here. You could return false in this case, or just ignore it. If you insist on giving some diagnostic for invalid enum values, then the validity check for this type should be done during input validation (i.e. in tox.c). At this point in the code, we can then statically prove that this is dead code.

For the proxy type, we check it in tox.c, for savedata type, we just ignore invalid values. I'll leave it up to you what to do here - just providing some thoughts.


toxcore/nat_traversal.c, line 222 at r6 (raw file):

    if ((traversal_type == TOX_TRAVERSAL_TYPE_UPNP) || (traversal_type == TOX_TRAVERSAL_TYPE_ALL)) {
        if (use_status) {
            status->upnp = upnp_map_port(log, proto, port);

You could make these 4 lines: upnp_map_port(log, proto, port, status ? &status->upnp : NULL);. Same for natpmp.


toxcore/nat_traversal.c, line 251 at r6 (raw file):

const char *str_nat_traversal_error(NAT_TRAVERSAL_STATUS status)
{
    char *err = calloc(MAX_ERR_LENGTH + 1, sizeof(char));

This allocation leaks on every call. Since you're not doing any string formatting, you could just return the string literal. Replace all strcpy+breaks with return "message";.


toxcore/tox.api.h, line 363 at r6 (raw file):

 * Type of technology used to try to traverse a NAT.
 */
enum class TRAVERSAL_TYPE {

What do you think about making this a bitfield instead of an enum class? It currently is a bitfield by accident: UPNP | NATPMP == ALL.

Note that this is a question, don't change things before discussing.


Comments from Reviewable

@Ansa89

This comment has been minimized.

Show comment
Hide comment
@Ansa89

Ansa89 Oct 31, 2016

IMHO traversal_type should be checked here as part of input arguments validation phase.
TOX_TRAVERSAL_TYPE can be a bitfield, but I never used them so expect bad/incorrect code.

Ansa89 commented Oct 31, 2016

IMHO traversal_type should be checked here as part of input arguments validation phase.
TOX_TRAVERSAL_TYPE can be a bitfield, but I never used them so expect bad/incorrect code.

Ansa89 added some commits Oct 31, 2016

@GrayHatter

@Ansa89 this needs to pass reviewable, and reviewable + github have mangled the comments so bad I can't review this.

Please reopen a new pull request and I'll review that one.

I'm going to leave this open for now to make sure I don't clobber any information.

@Ansa89 Ansa89 referenced this pull request Nov 1, 2016

Closed

Add NAT traversal #226

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Nov 2, 2016

Member

Closing in favour of #226.

Member

iphydf commented Nov 2, 2016

Closing in favour of #226.

@iphydf iphydf closed this Nov 2, 2016

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