IP4 strings validator

Question

Trying to write fast non dependency IP4 string validator for embedded system, It should counts leading zeros, values over 255 and other corrupted strings. Not sure if I missed something. Сan you guys take a look:

// Return 0 on error, store result to segs, if segs != 0. int is_valid_ip4(const char *str, unsigned char* segs) { int dots = 0; // Dot count. int chcnt = 0; // Character count within segment. int accum = 0; // Accumulator for segment. // Catch NULL pointer. if (str == 0) return 0; // Process every character in string. for (;;) { // Check numeric. if (*str < '0' || *str > '9') return 0; // Prevent leading zeros. if (chcnt > 0 && accum == 0) return 0; // Accumulate and check segment. accum = accum * 10 + *str - '0'; if (accum > 255) return 0; // Store result. if (segs) segs[dots] = (unsigned char)accum; // Advance other segment specific stuff. ++chcnt; ++str; // Finish at the end of the string when 3 points are found. if (*str == '\0' && dots == 3) return 1; // Segment changeover. if (*str == '.') { // Limit number of segments. ++dots; if (dots == 4) return 0; // Reset segment values. chcnt = 0; accum = 0; ++str; } } } 

Answer 1

<stdbool.h>

Unless you need compatibility with C89 for some reason, I would use bool for the return type of the function and true and false as the possible values.

It helps readability.

---

sscanf()

This one depends on your performance needs. A solution using sscanf() will be much easier to understand with a simple look and also shorter in code, but may also be much slower (a benchmark would be appropriate).

Update: I would discard sscanf() because it would accept spaces before the numbers; better use inet_pton() as @vnp suggested.

---

<stdint.h>

Same as with bool: segs seems at first glance to be some string, but after some time I realized it's just an array of 8-bit unsigned integers. You should use uint8_t for that.

In case of <stdint.h> I would say that if you don't have it, you should do the typedefs yourself (enclosed in some #if) anyway. Don't do that with bool, however, which is very dangerous.

---

Unneeded cast

unsigned char *p; int i; ... *p = (unsigned char)i; 

This cast is not needed.

Usually casts are very dangerous: they can hide bugs that otherwise the compiler would catch easily. Don't ever cast, unless you know a very good reason to.

---

BUG! (No, I was wrong)

// Check numeric. if (*str < '0' || *str > '9') return 0; 

This line returns 0 at the first dot it finds. The function doesn't work!

This if that comes later is always false due to that:

// Segment changeover. if (*str == '.') { /// Unreachable code!!! } 

---

<ctype.h>

There are some functions in <ctype.h> that you should use:

// Check numeric. if (*str < '0' || *str > '9') return 0; 

That code above can be simplified to this one (and the comment is now even more obvious and therefore removed ;-):

if (!isdigit((unsigned char)*str)) return 0; 

NOTES: The standards require that the argument c for these functions is either EOF or a value that is representable in the type unsigned char. If the argument c is of type char, it must be cast to unsigned char.

This is necessary because char may be the equivalent of signed char, in which case a byte where the top bit is set would be sign extended when converting to int, yielding a value that is outside the range of unsigned char.

See man isdigit.

If you're going to use these functions, I would create an unsigned char pointer to alias the char * parameter, so that you don't need to cast all the time.

Answer 2

I'll flag another issue that wasn't pointed out: it seems that the output array of segs (maybe rename it to segments?) is assumed to be allocated by the caller. Since there is no comment that describes that it needs to be of size 4, then the caller might send a smaller array and then the segs[dots] access would seg fault in the best case and overwrite memory silently in the worst case. I would either document that requirement or allocate the memory internally and ask the caller to delete[] it.

Answer 3

The first thing to improve the readability of the code is to define proper data types. Having an unsigned char * is confusing, both to humans and to the compiler.

#include <stdint.h> typedef struct { uint8_t part[4]; } ip4_address; 

Next, I found your code hard to follow because of the forever loop and the many if clauses. You do have a point though because you mention '0' and '9' and '.' only once in the code, which is good.

One thing I don't like is that you write unfinished parsing results to the returned IPv4 address. For example, when parsing 123.456, the returned IPv4 address would be 123.45.?.?. It's just the unnecessary memory access that I don't like. I prefer code that keeps the intermediate values in local variables and only writes them back when it is completely calculated.

I find it much simpler to read if the code is grouped by things from the application domain, which in this case is numbers and dots. I would write it like this:

#include <stdbool.h> #include <stdint.h> #include <stdio.h> typedef struct { uint8_t part[4]; } ip4_address; bool is_valid_ip4(const char *str, ip4_address *addr) { if (!str) return false; for (int i = 0; i < 4; i++) { if (!(*str >= '1' && *str <= '9')) return false; unsigned n = *str++ - '0'; if (*str >= '0' && *str <= '9') { n = 10 * n + *str++ - '0'; if (*str >= '0' && *str <= '9') { n = 10 * n + *str++ - '0'; if (n > 255) return false; } } addr->part[i] = n; if (i == 3) return *str == '\0'; if (*str++ != '.') return false; } return false; } int main(int argc, char **argv) { ip4_address addr; if (1 < argc && is_valid_ip4(argv[1], &addr)) { printf("%d.%d.%d.%d\n", addr.part[0], addr.part[1], addr.part[2], addr.part[3]); } else return 1; } 

An IPv4 address, in the usual syntax, consists of 4 numbers. This is expressed by the for loop that contains exactly 4 iterations.

An IPv4 address must start with a number. Therefore the upper part of the for loop is concerned with parsing and validating this number. The lower part handles the dots.

Organizing the code like this produces fewer jumps when stepping through it, and as a result, the execution position is simple to follow. Inside the for loop, it just goes straight from the top to the bottom. In your code, on the other hand, it jumps around much more often, which makes the code hard to follow.

Sure, my code repeats the character constants more often and is therefore more susceptible to typos, but finding these is the job of the test suite.

I took care of only defining the minimum necessary variables since keeping track of changing variable values is difficult.

As in your code, I explicitly test against the digits 0 to 9 instead of calling isdigit, just in case the code runs with an Arabic execution character set that defines some more digits. IPv4 addresses, according to the RFC, are written in ASCII digits.

I didn't write any comments in my code because I think that its structure is so simple and follows common practice that there is nothing surprising to it. By contrast, you wrote many comments, most of which are redundant.