Advice/help/better solution for checking whether a given string can be a valid IP address or not

Question

Honestly, I think the code which I've written is trash and I don't think it's the best way to solve the problem. I need a few suggestions or improvements to solve this problem. I'm still new to coding. Appreciate it if you can give some tips on how to work with strings and various string functions.

CONDITIONS FOR THE STRING TO BE AN IP ADDRESS:-

An identification number for devices connected to the internet. An IPv4 addresses written in dotted quad notation consists of four 8-bit integers separated by periods.

In other words, it's a string of four numbers each between 0 and 255 inclusive, with a "." character in between each number. All numbers should be present without leading zeros.

Examples:

192.168.0.1 is a valid IPv4 address
255.255.255.255 is a valid IPv4 address
280.100.92.101 is not a valid IPv4 address because 280 is too large to be an 8-bit integer (the largest 8-bit integer is 255)
255.100.81.160.172 is not a valid IPv4 address because it contains 5 integers instead of 4
1..0.1 is not a valid IPv4 address because it's not properly formatted
17.233.00.131 and 17.233.01.131 are not valid IPv4 addresses because they contain leading zeros

Here's my code (I know it's trash and doesn't make any sense):-

bool isIPv4Address(char * inputString) {
    int count = 0, period = 0;
    int k = 0, i = 0;
    int digit = 0;
    
     
    while(inputString[i] != '\0')
    { 
        count = 0;
        digit = 0;
        i = k;
        
        if(inputString[i] == '0' || inputString[i] == '.')
        {
            if(inputString[i+1] >47 && inputString[i+1] < 58)
            {
                return false;
            }
        }
        
        while(inputString[i] != '.' && inputString[i] != '\0')
        {
            if(inputString[i] < 48 || inputString[i] > 57)
            {
                return false;
            } 
            count += (int)inputString[i];
            i++;
            digit++;
        }
        
        if(digit == 3)
        {
            if(inputString[i - 3] > '2')
            {
                return false;
            }
        }
        
        if(inputString[i] == '.')
        {
            period++;
        }
        
        k = i+1;
      
        if(count < 48 || count > 156)
        {
            return false;
        }
        
        if(inputString[i] == '\0')
        {
            break;
        }
    }
    
    if(period == 3)
    {
        return true;
    }else
    {
        return false;
    }  
}

Two of the given examples fail, so it's a bit early for code review: 192.168.0.1 is reported as invalid, because the sum of the ASCII values of 192's digit exceeds 156 -- what is that test about? 280.100.92.101 is reported as valid, because checking whether the first of three digits is 3 or more is not enough to catch all numbers greater than 255. — M Oehm, Jun 11 '21 at 18:19
@MOehm the test is about checking whether the sum of ASCII values of the digits is greater than the sum of digits of 255's ASCII value or not. It's pointless ig — josh_3501, Jun 11 '21 at 18:33
On my network `192.168.0.1` is not a valid IP address. Looking at the syntax to determine if an address is valid is not enough. The validity can only be determined when you try to use the address. Trying to filter out bad addresses early based on syntax, may have a great risk of falsely rejecting good addresses, angering users. It may be better to trust that the user knows best, and fail when trying to use the data. If you really want to prefilter, use the same function to parse the address string for correct syntax and for communication. — HAL9000, Jun 11 '21 at 20:10
Oh, and according to wikipedia, the address `192.0.2.253`, may allso have the representations `0xC00002EB`, `0xC0.0x00.0x02.0xEB` or `0300.0000.0002.0353`. Writing parsers for poorly documented formats is always fun. — HAL9000, Jun 11 '21 at 20:17

score 5 · Answer 1 · answered Jun 11 '21 at 19:41

On a POSIX system, use the inet_addr() function:

NAME

inet_addr, inet_ntoa - IPv4 address manipulation

SYNOPSIS
#include <arpa/inet.h>

in_addr_t inet_addr(const char *cp);
char *inet_ntoa(struct in_addr in);
DESCRIPTION

The inet_addr() function shall convert the string pointed to by cp, in the standard IPv4 dotted decimal notation, to an integer value suitable for use as an Internet address.

Unless you're writing the code for practice, there's no need to reinvent an already-existing, standard solution.

...

RETURN VALUE

Upon successful completion, inet_addr() shall return the Internet address. Otherwise, it shall return ( in_addr_t)(-1).

...

score 3 · Accepted Answer · answered Jun 13 '21 at 14:31

I found this question on the code review site. Since the code doesn't work as expected the question doesn't fit on code review, so I will provide the review here instead.

Magic Numbers

This was mentioned in a previous answer:

It is not clear in the code what the numbers 47, 48, 59 and 156 mean.

You can't assume that the character set in use is ASCII, therefore it is safer to use a range check based on 0 to 9.

   if(inputString[i] == '0' || inputString[i] == '.')
   {
       if(inputString[i+1] < '0' && inputString[i+1] > '9')
       {
           return false;
       }
   }

It might be better to use the isdigit function provided by ctype.h

   if(inputString[i] == '0' || inputString[i] == '.')
   {
       if(!isdigit(inputString[i+1]))
       {
           return false;
       }
   }

Complexity

This code would be easier to read, write, and debug if the function were broken into smaller functions that had only one purpose. Each one of the inner loops or checks should be a function.

The art of programming is the art of breaking problems into smaller and smaller pieces until each piece is very easy to solve. There are programming principles such as the Single Responsibility Principle that describe this.

Use the C Library to Make the Checks easier

There are 2 functions that can convert strings to integers that would make checking for numbers above 255 easier. The first is atoi and the second is strtol, either of these functions will give you a number you can compare to 255.

Careful with `isdigit()` and plain (i.e. possibly signed) `char`! — Toby Speight, Jun 18 '21 at 15:00

score 2 · Answer 3 · answered Jun 11 '21 at 19:35

You had a lot of loose 47, 48, etc. values for things like '0'. Better to use the latter syntax.

There were a number of if range checks. Using some additional state variables can reduce the complexity.

Using inputString[i] everywhere is cumbersome. Better to do (e.g. int chr = inputString[i]; and use chr instead--it's simpler and easier to read).

The original program misidentified on:

192.168.0.1
280.100.92.101

I've refactored the code and built a diagnostic test program. It is annotated:

#include <stdio.h>
#include <stdbool.h>
#include <stdlib.h>

#define ERR(_expr) \
    if (err != NULL) \
        break; \
    if (_expr) { \
        err = #_expr; \
        break; \
    }

const char *bigerr;
int opt_i;

bool
isfix1(const char *inputString)
{
    int i = 0;
    int ndig = -1;
    int ndot = 0;
    int val = 0;
    int leading_zero = 0;
    unsigned char ipv[4];
    const char *err = NULL;

    while (1) {
        int chr = inputString[i++];
        int eos = (chr == 0);

        // check the last number we got
        if ((chr == '.') || eos) {
            ERR(ndig == 0);

            // we don't allow (because it might be decoded as octal):
            //   x.00.y.z x.01.y.z
            // but this must be ok:
            //   x.0.y.z
            ERR(leading_zero && (ndig > 1));

            // we can only have 3 dots in the string
            ndot += (! eos);
            ERR(ndot > 3);

            ndig = 0;
            val = 0;

            if (eos) {
                // we must have _exactly 3 dots in the string
                ERR(ndot != 3);
                break;
            }
            continue;
        }

        // collect digits
        if ((chr >= '0') && (chr <= '9')) {
            val *= 10;
            chr -= '0';
            val += chr;

            if (ndig < 0)
                ndig = 0;

            // remember whether substring started with "0"
            if (ndig == 0)
                leading_zero = (chr == 0);

            // we can only have up to 3 digits per byte
            ++ndig;
            ERR(ndig > 3);

            // max value for byte
            ERR(val > 255);

            // just for fun -- the binary IP address
            ipv[ndot] = val;
            continue;
        }

        // any invalid character
        ERR(chr != 0);
    }

    if (0)
        printf("IPV: %d.%d.%d.%d\n",ipv[0],ipv[1],ipv[2],ipv[3]);

    bigerr = err;

    return (err == NULL);
}

bool
isorig(const char *inputString)
{
    int count = 0,
        period = 0;
    int k = 0,
        i = 0;
    int digit = 0;

    while (inputString[i] != '\0') {
        count = 0;
        digit = 0;
        i = k;

        if (inputString[i] == '0' || inputString[i] == '.') {
            if (inputString[i + 1] > 47 && inputString[i + 1] < 58) {
                return false;
            }
        }

        while (inputString[i] != '.' && inputString[i] != '\0') {
            if (inputString[i] < 48 || inputString[i] > 57) {
                return false;
            }
            count += (int) inputString[i];
            i++;
            digit++;
        }

        if (digit == 3) {
            if (inputString[i - 3] > '2') {
                return false;
            }
        }

        if (inputString[i] == '.') {
            period++;
        }

        k = i + 1;

        if (count < 48 || count > 156) {
            return false;
        }

        if (inputString[i] == '\0') {
            break;
        }
    }

    if (period == 3) {
        return true;
    }
    else {
        return false;
    }
}

int
dofnc(bool (*fnc)(const char *),const char *ipaddr,const char *reason)
{
    int valid;

    bigerr = NULL;

    valid = fnc(ipaddr);
    printf("%s",valid ? "valid" : "INVALID");

    if (bigerr != NULL)
        printf(" (%s)",bigerr);

    printf(" (%s)\n",(valid != (reason == NULL)) ? "FAIL!" : "pass");

    return valid;
}

void
dotest(const char *ipaddr,const char *reason)
{
    static int sep = 0;

    if (sep)
        printf("\n");
    sep = 1;

    printf("IPADDR: %s",ipaddr);
    if (reason != NULL)
        printf(" -- %s",reason);
    printf("\n");

    int valid[2];
    printf("isorig: ");
    valid[0] = dofnc(isorig,ipaddr,reason);
    printf("isfix1: ");
    valid[1] = dofnc(isfix1,ipaddr,reason);

    do {
        if (opt_i)
            break;

        for (int idx = 0;  idx < 2;  ++idx) {
            if (valid[idx] != (reason == NULL))
                exit(1);
        }
    } while (0);
}

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

    --argc;
    ++argv;

    for (;  argc > 0;  --argc, ++argv) {
        char *cp = *argv;
        if (*cp != '-')
            break;

        cp += 2;
        switch (cp[-1]) {
        case 'i':
            opt_i = ! opt_i;
            break;
        }
    }

    dotest("192.168.0.1",NULL);
    dotest("255.255.255.255",NULL);
    dotest("280.100.92.101","280 is too large to be an 8-bit integer");
    dotest("255.100.81.160.172","contains 5 integers instead of 4");
    dotest("1..0.1","not properly formatted");
    dotest("1.0.1.","not properly formatted");
    dotest("17.233.00.131","contain leading zeros");
    dotest("17.233.01.131","contain leading zeros");

    return 0;
}

Here is the program output:

IPADDR: 192.168.0.1
isorig: INVALID (FAIL!)
isfix1: valid (pass)

IPADDR: 255.255.255.255
isorig: valid (pass)
isfix1: valid (pass)

IPADDR: 280.100.92.101 -- 280 is too large to be an 8-bit integer
isorig: valid (FAIL!)
isfix1: INVALID (val > 255) (pass)

IPADDR: 255.100.81.160.172 -- contains 5 integers instead of 4
isorig: INVALID (pass)
isfix1: INVALID (ndot > 3) (pass)

IPADDR: 1..0.1 -- not properly formatted
isorig: INVALID (pass)
isfix1: INVALID (ndig == 0) (pass)

IPADDR: 1.0.1. -- not properly formatted
isorig: INVALID (pass)
isfix1: INVALID (ndig == 0) (pass)

IPADDR: 17.233.00.131 -- contain leading zeros
isorig: INVALID (pass)
isfix1: INVALID (leading_zero && (ndig > 1)) (pass)

IPADDR: 17.233.01.131 -- contain leading zeros
isorig: INVALID (pass)
isfix1: INVALID (leading_zero && (ndig > 1)) (pass)

Advice/help/better solution for checking whether a given string can be a valid IP address or not

3 Answers3

Magic Numbers

Complexity

Use the C Library to Make the Checks easier