A simple NAT library

I've created a simple NAT library, with four essential functions:

• Find the UNPN device (the router in my case).

• Get the external IP address.

• Add a port forwarding entry.

• Delete a port forwarding entry.

Currently, updating an entry in the NAT table will change only the internal client ip address or the internal port, so to update an external port you have to delete it first then insert a new one.

TODO:

• Add other functionality (listing mapping entries, number of entries ...).

• Add support for other platforms.

• more testing if possible.

An example is included; just un-comment the part that you want to test. The library compiled and tested against Visual Studio 2008.

Any suggestions: optimization, coding style, bugs.

main.c:

```
#pragma comment(lib, "ws2_32.lib")

#include
#include
#include
#include

#if (_MSC_VER >= 1500)
#include
#endif

#define MALLOC(x) HeapAlloc(GetProcessHeap(), 0, (x))
#define FREE(x) HeapFree(GetProcessHeap(), 0, (x))

#define TCP_PROTOCOL 0
#define UDP_PROTOCOL 1

#define INVALID_ARGS 402
#define ACTION_FAILED 501
#define ENTRY_MAPPING_NOTEXIST 714
#define INVALID_REMOTEHOST_IP 715
#define INVALID_REMOTEHOST_PORT 716
#define ENTRY_CONFLICT 718
#define SAME_PORT_REQUIRED 724
#define ONLY_PERMANENT_LEASE 725
#define OWILDCARD_IP_REQUIRED 726
#define OWILDCARD_PORT_REQUIRED 727

int boradcastDiscovery(char *upnpDeviceIp);
int getExternalpAddress(char upnpDeviceIp, char localIp, char *externalIpAddress);
int addPortForwardEntry(char upnpDeviceIp, char localIp, int externalPort, int internalPort, int protocol, char internalp, char entryDescription);
int deletePortForwardEntry(char upnpDeviceIp, char localIp, int externalPort, int protocol);

int main() {
char buf[512],
ipAddress[16];
int errorCode = 0;

/*ZeroMemory(ipAddress, 16);
boradcastDiscovery(ipAddress);

printf("UPNP device IP: %s\n",

My main comment is that you need to concentrate on avoiding duplication of
code and on writing smaller functions. 50 lines or so is the sort of max length I use. You also use long names that are often
too long giving the code a very dense appearance and making it difficult ot
read. Smaller functions, reduced variable scope and hence shorter name sizes
will help here.

In function boradcastDiscovery, the name could be better as the current
(misspelt) name doesn't say what the function does. getGateway() perhaps?

In this function we have:

struct sockaddr_in upnpControl,
       broadcast_addr;

SOCKET sock = INVALID_SOCKET;
sock = socket(AF_INET, SOCK_DGRAM, 0);
if (sock == INVALID_SOCKET)
    return WSAGetLastError();

if(setsockopt(sock, SOL_SOCKET, SO_BROADCAST, searchIGDevice, sizeof(searchIGDevice)) == SOCKET_ERROR)
    return WSAGetLastError();

struct sockaddr_in upnpControl;
upnpControl.sin_family = AF_INET;
upnpControl.sin_port = htons(0);
upnpControl.sin_addr.s_addr = INADDR_ANY;
if (bind(sock, (sockaddr*)&upnpControl, sizeof(upnpControl)) == SOCKET_ERROR)
    return WSAGetLastError();

This could easily be extracted to a function:

static int
getBroadcastSocket(char *device, size_t size)
{
    SOCKET sock = socket(AF_INET, SOCK_DGRAM, 0);
    if (sock == INVALID_SOCKET) {
        return -1;
    }
    int status = setsockopt(sock, SOL_SOCKET, SO_BROADCAST, device, size);
    if (status != SOCKET_ERROR) {
        struct sockaddr_in s;
        s.sin_family = AF_INET;
        s.sin_port = htons(0);
        s.sin_addr.s_addr = INADDR_ANY;
        status = bind(sock, (sockaddr*)&s, sizeof s);
    }
    if (status == SOCKET_ERROR) {
        close(sock);
        return -1;
    }
    return sock;
}

Notice that sock is closed on error.

Another example is in code such as this which extracts a field from a string
and is repeated numerous times thoughout the whole code:

if(strstr(responseHeader, "\r\n\r\n")) {
    // Move the pointer to the first digit
    pLen = strstr(responseHeader, "Content-Length: ") + 16;
    ZeroMemory(pBodyLen, 5);
    // Get the body length
    while(*pLen != '\r') {
        pBodyLen[j] = *pLen;
        *pLen++;
        ++j;
    }

This should be extracted into a suitable function. Also of note in this code
are:

• the use of explicit lengths (16 and 5 here), which is bad practice;

• the lack of checks for target buffer overflow (often difficult);

• the use of variable j which was initialize far away at the start of the

function;

• inappropriate loop type - a for would be better.

A few other comments:

-
The address "239.255.255.250" would be better if extracted to a #define at the top
perhaps.

-
The code

char *proto = NULL,
...
proto = (char *)MALLOC(4);
ZeroMemory(proto, 4);
if(protocol == TCP_PROTOCOL)
    StringCbPrintf(proto, 4, "TCP");
else
    StringCbPrintf(proto, 4, "UDP");

would be better as

const char *proto = (protocol == TCP_PROTOCOL) ? "TCP" : "UDP";

-
Some of your calls to ZeroMemory often look redundant. The sizes in these
calls and in calls to StringCbPrintf are often explicit numbers rather than
using sizeof, which would be better.

-
The huge xml strings are distracting and would, I think, be better extracted
into suitably named functions, passing in the target buffer and the parameters
that need to be printed into the strings.

-
Variables should be defined close to their first point of use and initialised
on use if possible. For example there is no benefit in writing:

SOCKET sock = INVALID_SOCKET;
sock = socket(AF_INET, SOCK_STREAM, 0);

when you could write

SOCKET sock = socket(AF_INET, SOCK_STREAM, 0);

-
you are using TCP so you don't have to create a complete string before
sending it. For example instead of writing

createHeader(header, ...);
createRequest(request, ...);
concatenate(combined, header, request);
send(combined);

you can instead do:

createHeader(header, ...);
send(header);
createRequest(request, ...);
send(request);

Hopefully you will find something above that is of use :-)

StackExchange Code Review Q#35549, answer score: 9