Calculate IP Address

Question

I created this based on this website which calculates IP address information such as hosts, netmasks etc.

I'm quite new to C and I'm still grasping the idea of the whole array and pointers and bit manipulations. Code works the way I want it to work but I'd like to improve this. I would like to know how I did as well as what are some things I can look out for when programming in C.

#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <stdlib.h>
#include <math.h>

#define B1 0
#define B2 1
#define B3 2
#define B4 3
#define CIDR 4

void ConvertIpToInt(char address[], int address_block[]);

int main(int argc, char** argv) {

    int address_block[4], netmask_block[4];

    char address[] = "128.42.0.0/25";

    ConvertIpToInt(address, address_block);

    char literal_netmask_bin[32];
    int bit_counter, net_bit_counter = 0;
    while (bit_counter != 33) {
        while (net_bit_counter != address_block[CIDR]) {
            literal_netmask_bin[bit_counter] = '1';
            net_bit_counter += 1;
            bit_counter += 1;
        }
        literal_netmask_bin[bit_counter] = '0';
        bit_counter += 1;
    }

    int i, block_counter, sqN_value, sqN = 0;
    bit_counter = 0;
    for (i = 0; i < 33; i++) {
        bit_counter += 1;
        if (literal_netmask_bin[i] == '1') {
            sqN_value += pow(2, 7 - sqN);
        }
        sqN += 1;
        if (bit_counter == 8) {
            netmask_block[block_counter] = sqN_value;
            bit_counter = 0;
            sqN = 0;
            sqN_value = 0;
            block_counter += 1;
        }
    }

    int wildcard_netmask_block[4];
    i = 0;
    for (i = 0; i < 4; i++) {
        wildcard_netmask_block[i] = ~netmask_block[i] & 0xFF;
    }

    printf("> [GENERAL]\n\n");
    printf("Address:---------------------> %d.%d.%d.%d\n", address_block[B1], address_block[B2], address_block[B3], address_block[B4]);
    printf("Network Mask:----------------> %d.%d.%d.%d => %d\n", netmask_block[B1], netmask_block[B2], netmask_block[B3], netmask_block[B4], address_block[CIDR]);
    printf("Wildcard Mask:---------------> %d.%d.%d.%d\n", wildcard_netmask_block[B1], wildcard_netmask_block[B2], wildcard_netmask_block[B3], wildcard_netmask_block[B4]);
    printf("Network Address:-------------> %d.%d.%d.%d\n", address_block[B1] & netmask_block[B1], address_block[B2] & netmask_block[B2], address_block[B3] & netmask_block[B3], address_block[B4] & netmask_block[B4]);
    printf("Broadcast Address:-----------> %d.%d.%d.%d\n", wildcard_netmask_block[B1] | address_block[B1], wildcard_netmask_block[B2] | address_block[B2], wildcard_netmask_block[B3] | address_block[B3], wildcard_netmask_block[B4] | address_block[B4]);
    printf("Minimum Usable Address:------> %d.%d.%d.%d\n", address_block[B1] & netmask_block[B1], address_block[B2] & netmask_block[B2], address_block[B3] & netmask_block[B3], (address_block[B4] & netmask_block[B4]) + 1);
    printf("Maximum Usable Address:------> %d.%d.%d.%d\n", wildcard_netmask_block[B1] | address_block[B1], wildcard_netmask_block[B2] | address_block[B2], wildcard_netmask_block[B3] | address_block[B3], (wildcard_netmask_block[B4] | address_block[B4]) - 1);
    printf("Number of Hosts:-------------> %d\n", (int) pow(2, 32 - address_block[CIDR]) - 2);
    printf("Total Hosts:-----------------> %d\n\n", (int) pow(2, 32 - address_block[CIDR]));
}

void ConvertIpToInt(char address[], int address_block[]) {
    char * pch;
    char delimeters[] = "./";
    pch = strtok(address, delimeters);
    int i = 0;
    while (pch != NULL) {
        address_block[i] = strtol(pch, NULL, 10);
        i += 1;
        pch = strtok(NULL, delimeters);
    }
}

Answer 1

Initialize before using

    int bit_counter, net_bit_counter = 0;

There are a couple problems. First, if you declare and initialize in the same statement, you should only declare one variable.

    int bit_counter = 0;
    int net_bit_counter = 0;

If you have two variables, declare them on two lines. Only declare multiple variables on a single line if you initialize none of them. And some would argue not even then. There's no functional difference, but it is much easier to read with every initialized declaration on a separate line.

Second, you don't initialize bit_counter. C does not automatically initialize variables to default values, so this can cause bizarre behavior. When I first tried to run this code, it gave me a runtime error until I initialized all the variables.

Know your bounds

    while (bit_counter != 33) {

This is a bit fragile. You currently allow bit_counter to be incremented multiple times per iteration of this loop. What if when it is 32, you increment to 34 without hitting this check? The check will keep succeeding until the the program crashes (possibly due to trying to write outside the array).

    while (bit_counter <= 32) {

This is much more robust. It no longer has to hit the mark exactly to stop. Now it will stop for any value greater than 32, not just 33.

Another benefit, for readability, is that it now says 32, which is the critical number here. Previously it said 33, which is one more than the last number.

But this is actually a bug. It should be

    while (bit_counter < 32) {

because literal_bit_mask is only 32 long. The first element is 0. The last element is 31. Now it won't write past the end of the array.

C has an increment by one operator

            net_bit_counter += 1;
            bit_counter += 1;

You can write this more idiomatically as

            ++net_bit_counter;
            ++bit_counter;

This shouldn't make much functional difference (perhaps not any on many platforms), but most C programmers would find this more recognizable.

I used the prefix form because some C compilers may not be smart enough to avoid the copy step of the postfix form. I would expect most compilers to optimize that out, but we don't need to rely on that. It's unlikely to make a noticeable difference here.

++bit_counter increments bit_counter and returns that value. bit_counter++ returns the value it had before incrementing. If you don't use the value (as you don't here), there is no real functional difference. It's possible that the compiler may return different machine instructions. In particular, it might copy the value to another register and then increment. That's unnecessary if the

The following are functionally equivalent (leave the variables with the same values at the end):

int var = bit_counter++;

and

int var = bit_counter;
bit_counter++;

and

int var = bit_counter;
++bit_counter;

The following are different from the previous three, but are functionally equivalent to each other:

bit_counter++;
int var = bit_counter;

and

++bit_counter;
int var = bit_counter;

and

int var = ++bit_counter;

If bit_counter starts as 0 and is incremented to 1, then the first three will set var to 0. The last three will set var to 1.

Many consider it easier to read if the variables are changed on separate lines. This makes it obvious that the variables are changing. Doing two things on the same line is less obvious, because people may see one and miss the other.

Answer 2

Warnings and errors

A good compiler will alert you to some undefined behaviour in your code.

$ clang -Wall -c cr14674.cpp
cr14674.cpp:26:35: warning: array index 4 is past the end of the array (which
      contains 4 elements) [-Warray-bounds]
        while (net_bit_counter != address_block[CIDR]) {
                                  ^             ~~~~
cr14674.cpp:17:5: note: array 'address_block' declared here
    int address_block[4], netmask_block[4];
    ^
cr14674.cpp:60:142: warning: array index 4 is past the end of the array (which
      contains 4 elements) [-Warray-bounds]
  ...netmask_block[B3], netmask_block[B4], address_block[CIDR]);
                                           ^             ~~~~
cr14674.cpp:17:5: note: array 'address_block' declared here
    int address_block[4], netmask_block[4];
    ^
cr14674.cpp:66:69: warning: array index 4 is past the end of the array (which
      contains 4 elements) [-Warray-bounds]
  ...Hosts:-------------> %d\n", (int) pow(2, 32 - address_block[CIDR]) - 2);
                                                   ^             ~~~~
cr14674.cpp:17:5: note: array 'address_block' declared here
    int address_block[4], netmask_block[4];
    ^
cr14674.cpp:67:71: warning: array index 4 is past the end of the array (which
      contains 4 elements) [-Warray-bounds]
  ...%d\n\n", (int) pow(2, 32 - address_block[CIDR]));
                                ^             ~~~~
cr14674.cpp:17:5: note: array 'address_block' declared here
    int address_block[4], netmask_block[4];
    ^
cr14674.cpp:44:27: warning: variable 'block_counter' is uninitialized when used
      here [-Wuninitialized]
            netmask_block[block_counter] = sqN_value;
                          ^~~~~~~~~~~~~
cr14674.cpp:35:25: note: initialize the variable 'block_counter' to silence this
      warning
    int i, block_counter, sqN_value, sqN = 0;
                        ^
                         = 0
cr14674.cpp:25:12: warning: variable 'bit_counter' is uninitialized when used
      here [-Wuninitialized]
    while (bit_counter != 33) {
           ^~~~~~~~~~~
cr14674.cpp:24:20: note: initialize the variable 'bit_counter' to silence this
      warning
    int bit_counter, net_bit_counter = 0;
                   ^
                    = 0
6 warnings generated.

In fact, on my system, your program crashes with a segmentation fault. Accessing address_block[4] is out-of-bounds access, since the last member of a 4-element array is address_block[3]. That indicates that you need to declare int address_block[5], not int address_block[4].

It also helpfully tells you that bit_counter and block_counter need to be initialized to 0.

IP address representation

Representing an IPv4 address as an array of 4 ints makes manipulating the bits cumbersome. For analysis and manipulation, it's more helpful to think of it as one 32-bit number. The "dotted quad" notation is just a presentation format that humans find convenient.

Parsing

The parsing is too lenient. It will happily accept "127/0/0.1.8" just the same as "127.0.0.1/8". In fact, there is no error-reporting mechanism for invalid input. (By the way, "delimiters" is misspelled.)

The ConvertIpToInt() function feels odd to me. Extracting an array of five integers from a string is really just doing half the job, and still leaves your main() function with the task of building a representation of the netmask.

Using strtok() is not ideal, as it writes NUL characters into the string that was handed to it. I would just take advantage of the second argument to strtol() help you move along to the next element in the string.

void ConvertIpToInt(char address[], int address_block[]) {
    char *pch = address;
    for (int i = 0; i < 4 && pch; i++) {
        address_block[i] = strtol(pch, &pch, 10);
        if ((i < 4 && *pch == '.') || (*pch == '/')) {
            pch++;      /* Skip three dots, then a slash */
        } else {
            return;     /* Invalid input (should return an error code) */
        }   
    }   
    address_block[4] = strtol(pch, NULL, 10);
}

Calculations

This is a lot of code to build the netmask. There shouldn't be a need to build a 32-character string of '1' and '0' characters.

char literal_netmask_bin[32];
int bit_counter = 0, net_bit_counter = 0;
while (bit_counter != 33) {
    while (net_bit_counter != address_block[CIDR]) {
        literal_netmask_bin[bit_counter] = '1';
        net_bit_counter += 1;
        bit_counter += 1;
    }
    literal_netmask_bin[bit_counter] = '0';
    bit_counter += 1;
}

int i, block_counter = 0, sqN_value, sqN = 0;
bit_counter = 0;
for (i = 0; i < 33; i++) {
    bit_counter += 1;
    if (literal_netmask_bin[i] == '1') {
        sqN_value += pow(2, 7 - sqN);
    }
    sqN += 1;
    if (bit_counter == 8) {
        netmask_block[block_counter] = sqN_value;
        bit_counter = 0;
        sqN = 0;
        sqN_value = 0;
        block_counter += 1;
    }
}

Unfortunately, it's wrong. For a /24, I get 256.255.255.0 instead of 255.255.255.0.

There should be a very simple solution using bit-shifting.

---

printf("Total Hosts:-----------------> %d\n\n", (int) pow(2, 32 - address_block[CIDR]));

pow(2, …) is very much a bit-shifting calculation — you're just using a floating-point operation to do it. (See my other answer).

When a network is as small as /31 or /32, the minimum, maximum, and number of hosts doesn't really make sense anymore. You might want to suppress those.

Answer 3

Reinventing the wheel

Parsing CIDR notation is a somewhat common task, and you are reinventing-the-wheel. In fact, there is an inet_net_pton(3) function that does exactly that, available in BSD (including macOS), Linux, and other Unix flavours.

The inet_net_pton() function converts a presentation format Internet network number (that is, printable form as held in a character string) to network format (usually a struct in_addr or some other internal binary representation, in network byte order). It returns the number of bits, either computed based on the class, or specified with /CIDR), or -1 if a failure occurred (in which case errno will have been set. It will be set to ENOENT if the Internet network number was not valid).

Windows does not have an inet_net_pton() function, but it does support the inet_pton() POSIX function, which parses the address but not the netmask.

Even if you want to reinvent the wheel for fun or out of necessity, it pays to see how it is conventionally done, because:

• You may learn lessons from other programmers' design decisions.
• Other programmers might have an easier time figuring out your code if it follows the conventional approach.
• Following the standard interface lets you create interoperable code.

In particular, using a struct in_addr to represent an IPv4 address lets you actually use the parsed result in code that really makes network connections. (A struct in_addr just contains a 32-bit unsigned integer in big-endian format.)

Here is a solution based on inet_net_pton(). I think you would agree that it's simpler. Notice that each calculation works on the entire address at once, and each result is easily formatted as a dotted quad using inet_ntop().

#include <inttypes.h>
#include <stdio.h>
#include <arpa/inet.h>
#include <netinet/in.h>
#include <sys/types.h>

/**
 * Parses a string in CIDR notation as an IPv4 address and netmask.
 * Returns the number of bits in the netmask if the string is valid.
 * Returns -1 if the string is invalid.
 */
int parse_cidr(const char *cidr, struct in_addr *addr, struct in_addr *mask) {
    int bits = inet_net_pton(AF_INET, cidr, addr, sizeof addr);
    /* Shifting by 32 bits is undefined (http://stackoverflow.com/q/2648764) */
    mask->s_addr = htonl(~(bits == 32 ? 0 : ~0U >> bits));
    return bits; 
}   

/**
 * Formats the IPv4 address in dotted quad notation, using a static buffer.
 */
const char *dotted_quad(const struct in_addr *addr) {
    static char buf[INET_ADDRSTRLEN];
    return inet_ntop(AF_INET, addr, buf, sizeof buf);
}   

int main(int argc, char *argv[]) {
    struct in_addr addr, mask, wildcard, network, broadcast, min, max;
    int64_t num_hosts;

    int bits = parse_cidr(argv[1], &addr, &mask);
    if (bits == -1) {
        fprintf(stderr, "Invalid address/netmask: %s\n", argv[1]));
        return 1;
    }

    wildcard = mask;    wildcard.s_addr = ~wildcard.s_addr;
    network = addr;     network.s_addr &= mask.s_addr;
    broadcast = addr;   broadcast.s_addr |= wildcard.s_addr;
    min = network;      min.s_addr = htonl(ntohl(min.s_addr) + 1);
    max = broadcast;    max.s_addr = htonl(ntohl(max.s_addr) - 1);
    num_hosts = (int64_t)ntohl(broadcast.s_addr) - ntohl(network.s_addr) + 1;

    printf("> [GENERAL]\n\n");
    printf("Address:---------------------> %s\n", dotted_quad(&addr));
    printf("Network Mask:----------------> %s => %d\n", dotted_quad(&mask), bits);
    printf("Wildcard Mask:---------------> %s\n", dotted_quad(&wildcard));
    printf("Network Address:-------------> %s\n", dotted_quad(&network));
    printf("Broadcast Address:-----------> %s\n", dotted_quad(&broadcast));
    if (num_hosts > 2) {
        printf("Minimum Usable Address:------> %s\n", dotted_quad(&min));
        printf("Maximum Usable Address:------> %s\n", dotted_quad(&max));
        printf("Number of Hosts:-------------> %" PRId64 "\n", num_hosts - 2);
    }   
    printf("Total Hosts: ----------------> %" PRId64 "\n", num_hosts);
    return 0;
}