Why do I get a segmentation fault? The code works fine in CLion and VSC

Question

I am currently using exercism, to improve my programming skills. The code works fine in my IDE, but causes an error message on the website. What did I do wrong and how would you improve my code? Thx in advance :)

make: *** [makefile:22: test] Segmentation fault (core dumped)

#include "isogram.h"
#include <string.h>
#include <ctype.h>

bool is_isogram(const char phrase[]) {
    int length = strlen(phrase);
    char temp[length];
    
    for (int i = 0; i < length; i++)
        temp [i] = phrase[i];

    temp[length] = '\0';

    for(int i = 0; temp[i]; i++){
        temp[i] = tolower(temp[i]);
    }

    for(int i = 0; i < length; i++){
        for(int j = i+1; j < length;j++) {
            if (temp[i] == temp[j] && temp[i] != ' ' && temp[i] != '-')
                return false;
        }
    }
    return true;
}

int main(void) {
    char phrase[] = "lumberjacks";
    if (is_isogram(phrase))
        printf("true");
    else
        printf("false");
    return 0;
}

I asked ChatGPT and it suggested the temp[length] = '\0'; line, but this doesn't fix the issue.

Answer 1

For starters using the variable length array

int length = strlen(phrase);
char temp[length];

is unsafe and redundant. Pay attention to that the return type of the function strlen is the unsigned type size_t not the signed type int.

Also calling the function strlen is redundant. You can scan a string based on the fact that it is terminated by the terminating zero character '\0'.

You defined an array that does not have a room for the terminating zero character '\0'. As a result this assignment

temp[length] = '\0';

invokes undefined behavior.

To convert all characters of the variable length array before checking whether the string is isogram

for(int i = 0; temp[i]; i++){
    temp[i] = tolower(temp[i]);
}

is inefficient.

Words in a phrase can be separated not only by the space character ' ' but also by the tab character '\t'. Also you should exclude punctuations.

I would write the function the following way as shown in the demonstration program below.

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

bool is_isogram( const char phrase[] )
{
    bool isogram = true;

    if (*phrase != '\0')
    {
        for (const char *current = phrase + 1; isogram && *current != '\0'; ++current)
        {
            if (!isblank( ( unsigned char )*current ) && !ispunct( ( unsigned char )*current ))
            {
                char c = tolower( ( unsigned char )*current );

                const char *prev = current;

                while (isogram && prev != phrase)
                {
                    --prev;
                    isogram = tolower( ( unsigned char )*prev ) != c;
                }
            }
        }
    }  

    return isogram;
}

int main( void )
{
    const char *phrase = "lumberjacks";

    printf( "The phrase \"%s\" is isogram: %s\n",
        phrase, is_isogram( phrase ) ? "true" : "false" );
}

The program output is

The phrase "lumberjacks" is isogram: true

Answer 2

Vlad from Moscow has nicely pointed out the issues with your array length, but as he notes, the array you're using is unecessary. An O(n) solution this problem might involve creating a histogram: we're going to count each letter using a 26 element array where each element corresponds to a letter of the English alphabet. This can then be checked in one iteration to ensure no letter occurs more than once (or that every letter occurs the same number of times).

We could be potentially more efficient by doing this in one function, but this demonstrates how a program can be broken down into smaller problems.

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

int *alpha_histogram(const char *str) {
    int *hist = calloc(sizeof(int), 26);
    if (!hist) return NULL;

    for (const char *ch = str; *ch; ch++) {
         if (!isalpha(*ch)) continue;

         hist[tolower(*ch) - 'a']++;
    }

    return hist;
}

bool is_isogram(const char *str) {
    int *hist = alpha_histogram(str);
    if (!hist) exit(EXIT_FAILURE);

    for (size_t i = 0; i < 26; ++i) {
         if (hist[i] > 1) return false;
    }

    free(hist);

    return true;
}

int main(void) {
    const char *str = "lumberjacks";

    if (is_isogram(str)) {
        printf("\"%s\" is an isogram.\n", str);
    }
}