LeetCode 1108: Defanging an IP Address

Question

I'm posting two solutions for LeetCode's "Defanging an IP Address". If you'd like to review, please do. Thank you!

Problem

Given a valid (IPv4) IP address, return a defanged version of that IP address.

A defanged IP address replaces every period "." with "[.]".

Example 1:

Input: address = "1.1.1.1"
Output: "1[.]1[.]1[.]1"

Example 2:

Input: address = "255.100.50.0"
Output: "255[.]100[.]50[.]0"

Constraints:

• The given address is a valid IPv4 address.

Code 1

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

static const char* defangIPaddr(
    char* address
) {
    char* ipv4_memory = calloc(1, sizeof("###[.]###[.]###[.]###"));
    char* defanged = ipv4_memory;

    for (char* character = address; *character; character++) {
        if (*character == '.') {
            *ipv4_memory++ = '[';
            *ipv4_memory++ = '.';
            *ipv4_memory++ = ']';

        } else {
            *ipv4_memory++ = *character;
        }
    }

    return defanged;
}


int main() {

    printf ("%s \n", defangIPaddr("1.1.1.1"));
    printf ("%s \n", defangIPaddr("255.100.50.0"));

    return 0;
}

Code 2 without changing the input:

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

static const char* defangIPaddr(
    const char* address
) {
    char* cloned_address = NULL;
    cloned_address = strdup(address);

    char* ipv4_memory = calloc(1, sizeof("###[.]###[.]###[.]###"));
    char* defanged = ipv4_memory;


    for (char* character = cloned_address; *character; character++) {
        if (*character == '.') {
            *ipv4_memory++ = '[';
            *ipv4_memory++ = '.';
            *ipv4_memory++ = ']';

        } else {
            *ipv4_memory++ = *character;
        }
    }

    return defanged;
}


int main() {

    printf ("%s \n", defangIPaddr("1.1.1.1"));
    printf ("%s \n", defangIPaddr("255.100.50.0"));

    return 0;
}

Answer 1

Watch your memory allocations and deallocations. In both cases, you've got defangIPaddr returning a const char * to heap-allocated memory, which needs to be freed by the caller... but it can't be freed, because free expects a non-const void* as its argument.

Functions that return ownership-of-a-heap-allocation to the caller should (A) return char*, not const char*, and (B) be clearly documented.

In Code 2, you're not just leaking the returned pointer; you're also leaking the strdup'ed original pointer. There's no reason to call strdup here.

What you want is simply

char *defangIPaddr(const char *address) {
    char *defanged = calloc(sizeof("###[.]###[.]###[.]###"), 1);
    char *q = defanged;
    for (const char *p = address; *p != '\0'; ++p) {
        if (*p == '.') {
            *q++ = '[';
            *q++ = '.';
            *q++ = ']';
        } else {
            *q++ = *p;
        }
    }
    return defanged;
}

I've shortened your names character and ipv4_memory to just p and q. My p is for "pointer"; it's not a character at all, so character is a misleading name for it. q is simply what comes after p.

You got the arguments to calloc in the wrong order. In practice this doesn't matter; but getting it correct is easy and free. If you're unsure about an interface, check the man page.

In your original code, you did this:

char* cloned_address = NULL;
cloned_address = strdup(address);

That's a verbose way of writing

char* cloned_address = strdup(address);

Always initialize your variables — but initialize them to the right values! Don't initialize to a nonsense or garbage value and then later assign the right value over it; just initialize with the right value to begin with.

---

sizeof("###[.]###[.]###[.]###") is a mildly sneaky way of computing the maximum possible length. It might be better to allocate simply 3*strlen(address)+1 bytes, so that you can't possibly buffer-overflow, even if the caller passes in ".................." as their input.

Alternatively, you could compute the exact right buffer length and malloc exactly that much.

    int len = 0;
    for (const char *p = address; *p != '\0'; ++p) {
        len += (*p == '.') ? 3 : 1;
    }
    char *defanged = calloc(len+1, 1);
    [...]

---

Consider throwing in an if (defanged == NULL) return NULL; just in case the allocation fails.

Answer 2

Consider using asprintf()

Just like you are using strdup() to simplify making a copy of a string, consider using asprintf() to print a string without having to worry about allocating memory yourself. This will greatly simplify your code:

char* defangIPaddr(const char* address)
{
    char* defanged;
    int ip[4];

    if (sscanf(address, "%d.%d.%d.%d", &ip[0], &ip[1], &ip[2], &ip[3]) != 4)
        return NULL;

    if (asprintf(&defanged, "%d[.]%d[.]%d[.]%d", ip[0], ip[1], ip[2], ip[3]) == -1)
        return NULL;

    return defanged;
}

Not all platforms support asprintf() though, and this solution has worse performance than your solution, but on the other hand it's simpler (no for-loops or manual memory allocation necessary), and can be adapted easily to other problems where you have to transform an input string to an output string, as long as they can easily be represented by format strings.

Answer 3

Here is my take on the problem, a function which accepts any string length and only allocates exactly enough memory:

//#include <malloc/_malloc.h>
#include <stdlib.h> // malloc, free
#include <assert.h> // assert
#include <stdio.h>  // printf, fprintf

/**
 * "Defangs" a string by replacing all `.` with `[.]`.
 *
 * @param str The string to defang
 * @return A pointer to a heap-allocated, defanged version of the string. The
 * pointer may be `NULL` if the allocation failed. The caller is responsible for
 * freeing the pointed-to memory (via `free()`) when they are done with it.
 */
char* defang(const char* str) {
    size_t str_len = 0; // without null terminator
    size_t new_len = 1; // with null terminator

    for (const char* c = str; *c; c++) {
        str_len++;

        if (*c == '.') {
            new_len += 3;
        } else {
            new_len++;
        }
    }

    char* new = malloc(new_len);

    if (new) {
        char* ptr = new;
        for (size_t i = 0; i < str_len; i++) {
            const char* c = &str[i];

            if (*c == '.') {
                *ptr++ = '[';
                *ptr++ = '.';
                *ptr++ = ']';
            } else {
                *ptr++ = *c;
            }
        }

        *ptr++ = 0;

        assert(ptr - new == new_len);
    }

    return new;
}

int main() {
    char* defanged1 = defang("1.1.1.1");
    if (defanged1) {
        printf("%s\n", defanged1);
        free(defanged1);
    } else {
        fprintf(stderr, "Failed to allocate memory for defanged1\n");
        return 1;
    }

    char* defanged2 = defang("255.100.50.0");
    if (defanged2) {
        printf("%s\n", defanged2);
        free(defanged2);
    } else {
        fprintf(stderr, "Failed to allocate memory for defanged2\n");
        return 1;
    }

    return 0;
}

When memory cannot be allocated, it returns a null pointer to the caller to allow them to handle the situation, rather than attempting to continue and causing a segfault immediately.

Additionally, the function has a documentation comment which explains exactly what it does, what it takes, what it returns and what is expected of the caller. In my opinion, those four elements are essential to any documentation.

Then, the included main function is an example of how one would use the defang function, freeing the memory if it was allocated and failing with an error message if it was not.

Feel free to look over this code as an example of how to apply the advice that others have given.