9
\$\begingroup\$

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;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ The (very good) reviews notwithstanding, the code is exceptionally clean — honestly well done! I’m not sure I understand why you think the two implementations materially differ: neither changes the input, and you seem to have actually noticed that yourself. \$\endgroup\$ – Konrad Rudolph Oct 24 at 13:47
  • 2
    \$\begingroup\$ I don't see either function storing through the char* address arg, so neither one modifies its input. You could change it to const char *address and it would still compile. Some answers made that point implicitly, but I didn't see a clear mention of that important point that seems to be a sign of some kind of misunderstanding. (Although it seemed too small to post a separate answer.) \$\endgroup\$ – Peter Cordes Oct 25 at 9:31
12
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ You should check whether calloc() returned NULL. \$\endgroup\$ – G. Sliepen Oct 24 at 9:26
8
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ I have to strongly disagree with this suggestion. It is preferable to write portable code unless you are already constrained to that particular platform for some reason (like hardware dependencies). Relying on Gnu-specific functions just doesn't make sense when you can write portable C almost just as easily. \$\endgroup\$ – Cody Gray Oct 25 at 10:58
  • 1
    \$\begingroup\$ @CodyGray We can agree to disagree :) I don't like ignoring a perfectly good function just for the sake of portability. I've seen too many mistakes with people mucking about with strlen()+malloc()+strcpy() when they could've just used strdup(). And if it turns out your platform doesn't support those functions, you can just write a drop-in replacement for that platform. Of course only do this if the non-portable function you want to use is a very good fit for your problem in the first place. \$\endgroup\$ – G. Sliepen Oct 25 at 11:30
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 4
    \$\begingroup\$ Using new as a var name make it less convenient to port to C++ (where it's a keyword). If you regularly work with both languages, usually good to avoid that. Not that you should make your C worse so it can actually compile as C++, e.g. don't cast malloc, but otherwise-neutral things you might as well do. \$\endgroup\$ – Peter Cordes Oct 25 at 9:28
  • \$\begingroup\$ You would not have the new problem if you used the much-maligned Hungarian notation, pNew. :-) \$\endgroup\$ – Cody Gray Oct 25 at 10:59
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and why it is better than the original) so that the author and other readers can learn from your thought process. \$\endgroup\$ – Mast Oct 25 at 12:08
  • \$\begingroup\$ @Mast You're right, I did not review the original code. This answer was intended to add to the existing reviews, not necessarily stand on its own as a review. There is a doc comment though, those are important so I did detail that at least. ;) \$\endgroup\$ – Dev Oct 26 at 14:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.