3

So I'm doing this codewars kata and I'm succeding in every test but in the end the result fails because my code has a segmentation fault and I don't think I know enough about the language to find it! Can someone please halp?

int is_valid_ip(const char *addr) 
{
    char set[] = "1234567890";
    int current;
    int octet_counter = 0;
    char *octet = 0;
    octet = strtok(addr, ".");
    while (octet)
    {
        if (strspn(octet, set) != strlen(octet)) return 0; // checks for spaces
        if (strlen(octet) > 1 && (octet[0]) == '0') return 0; // checks for preceding zeros
        sscanf(octet, "%d", &current);
        if (current < 0 || current > 255) return 0; // checks for range
        octet = strtok(0, ".");
        ++octet_counter;
    }
    if (octet_counter == 4) return 1; // checks for number of octets
    return 0;
};

My code was kind of cleaner but after so much messing around trying to solve this problems it's become this...

void-main-void
  • 65
  • 7
  • 5
    You must not pass `const char *addr` to `strtok` because it modifies the string. MSVC issues a warning about this. If the string is physically read-only you'll get a fault. – Weather Vane Mar 17 '20 at 16:11
  • The site uses clang, and I usually wouldn't do that. The site requires you to do so. But I'll modify that, I'll just copy addr to a `char *ip` so it wont mess with the addr value! That won't solve the problem though! :( – void-main-void Mar 17 '20 at 16:14
  • 2
    You should find the length of the string and allocate `len+1` bytes and copy it, and finally `free` the memory. Aside: if you mean `NULL` use that, not `0`. – Weather Vane Mar 17 '20 at 16:16
  • Simpler solution: don't use strtok. – rici Mar 17 '20 at 16:17
  • Please show how you obtain the string argument which is passed. Ideally, post the [Minimal Reproducible Example](https://stackoverflow.com/help/minimal-reproducible-example) that shows the complete code. – Weather Vane Mar 17 '20 at 16:19
  • 2
    You scan each octet three times: once with strtok, again with strspn, and finally with scanf. That's twice too many. Take a look at `strtod`. – rici Mar 17 '20 at 16:21
  • Thanks guys. I was just focusing on making it work first, turns out copying the `addr` to a new `ip` variable solved the thing! – void-main-void Mar 17 '20 at 16:27
  • 1
    Possible Duplicate [C's strtok() and read only string literals](https://stackoverflow.com/questions/272876/cs-strtok-and-read-only-string-literals) – David C. Rankin Mar 17 '20 at 16:29
  • @rici Loved this `strtod` thing, I didn't know it existed! Thank you! – void-main-void Mar 17 '20 at 16:34

2 Answers2

4

As strtok() modifies the string to be tokenized and addr is defined as const char * (I assume this is a requirement) you may make a copy of the input string *addr:

char ip[16]; // enought to hold nnn.nnn.nnn.nnn
if(strlen(addr)>15) return 0;
strcpy(ip, addr);

subsequently operate on ip instead of addr

Or... avoid using strtok and parse/scan the string without modifying it.

Paolo
  • 15,233
  • 27
  • 70
  • 91
0

Here's a dumb solution using sscanf, just to show that it's possible. (Below, there are some better solutions.)

#include <stdio.h>

/* True if its argument represents a decimal number between
 * 0 and 255 without leading zeros. Assumes the characters are
 * all digits and that there are at most three of them.
 */
static int good(const char* octet) {
  switch (octet[0]) {
    case '0': return octet[1] == 0;
    case '1': return 1;
    case '2': return octet[1] == 0 || octet[2] == 0
              || octet[1] < '5'
              || octet[1] == '5' && octet[2] < '6';
    default:  return octet[1] == 0 || octet[2] == 0;
  }
}          

struct OctetSep { char oct[4], sep[2] };
int is_valid_ip(const char *addr) {
  struct OctetSep bits[4];
  /* The last %c conversion is expected to fail. */
  return sscanf(addr, "%3[0-9]%1[.]%3[0-9]%1[.]%3[0-9]%1[.]%3[0-9]%1c",
                      bits[0].oct, bits[0].sep,
                      bits[1].oct, bits[1].sep,
                      bits[2].oct, bits[2].sep,
                      bits[3].oct, bits[3].sep) == 7
         && good(bits[0].oct) && good(bits[1].oct)
         && good(bits[2].oct) && good(bits[3].oct);
}

Of course, the bulk of the work is being done by good, and it would be easy to make it a bit more general:

#include <stddef.h>

/* This idiocy is avoid problems with signed characters */
static int my_isdigit(char c) { return c >= '0' && c <= '9'; }

/* Returns the address of the first unmatched character after a match
 * of an integer with no leading zeros and maximum value 255.
 * This address may be in the middle of the integer, if it is too big.
 * If the string doesn't start with a digit, returns NULL.
 */
static const char* good(const char* octet) {
  switch (octet[0]) {
    case '0': return octet + 1;
    case '1': return   !my_isdigit(octet[1]) ? octet + 1
                     : !my_isdigit(octet[2]) ? octet + 2
                     : octet + 3;
    case '2': return   !my_isdigit(octet[1]) ? octet + 1
                     : !my_isdigit(octet[2]) ? octet + 2
                     : octet[1] < '5' || octet[1] == '5' && octet[2] < '6'
                                             ? octet + 3
                     : octet + 2;
    case '3': case '4': case '5': case '6': case '7': case '8':
    case '9': return !my_isdigit(octet[1]) : octet + 1 ? octet + 2;
    default:  return NULL;
  }
}

int is_valid_ip(const char *addr) {
  for (const char* i = "..."; (addr = good(addr)); ++addr, ++i) {
    if (*addr != *i) break;
    if (!*i) return 1;
  }
  return 0;
}

But probably the simplest code comes from the use of strtol, although you still need a bunch of manual checks. (This solution scales much better to applications where the valid integer range is larger than 256.)

#include <stdlib.h>

/* This idiocy is avoid problems with signed characters */
static int my_isdigit(char c) { return c >= '0' && c <= '9'; }

/* Returns the address of the first character following the integer
 * starting precisely at the supplied address, provided the integer
 * has no leading zeros and a maximum value of 255. Otherwise,
 * returns NULL.
 */
static const char* good(const char* octet) {
  if (*octet == '0') return octet + 1;
  /* This check is necessary because strtol also accepts whitespace and - */
  if (!my_isdigit(*octet)) return NULL;
  char *endptr;
  long value = strtol(octet, &endptr, 10);
  /* We already handled 0, so value==0 must be an error return */
  if (value <= 0 || value > 255) return NULL;
  return endptr;
}

int is_valid_ip(const char *addr) {
  for (const char* i = "..."; (addr = good(addr)); ++addr, ++i) {
    if (*addr != *i) break;
    if (!*i) return 1;
  }
  return 0;
}
rici
  • 234,347
  • 28
  • 237
  • 341