"Why doesn't my code work?" — to anyone learning the art of programming and writing to the Stack Overflow community

28 Jui 2022

Author: Andrey Karpov

Stack Overflow is full of questions from people learning to write code. Here's a tip: you can get answers to most of these questions if you run a static code analyzer against your code. That's so much faster!

A few days ago I was browsing Stack Overflow and stumbled upon an interesting discussion: "Segmentation fault when converting char * to char **". The author is learning how to code and wants to know what's wrong with the code.

#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>

char **get_words(char *buffer, char delimiter)
{
    printf("buffer = %s\n", buffer);
    char **words = malloc(sizeof(char *) * 100);
    if (words == NULL) {
        printf("Malloc Error\n");
        exit(84);
    }
    for (int i = 0; i < 100; i++) {
        words[i] = malloc(sizeof(char) * 100);
        if (words[i] == NULL) {
            printf("Malloc Error\n");
            exit(84);
        }
    }
    int word_count = 0;
    int l = 0;
    for (int i = 0; buffer[i] != '\0' && buffer[i]  != '\n'; i++, l++) {
        if (buffer[i] == delimiter) {
            words[word_count][l] = '\0';
            word_count++;
            l = -1;
        }
        else
            words[word_count][l] = buffer[i];
    }
    words[word_count][l] = '\0';
    return (words);
}

int main()
{
    char *buffer = malloc(sizeof(char) * 100);
    buffer = "hello world !\n";
    char **words = get_words(buffer, ' ');
    printf("words[0]= %s\n", words[0]);
    free (buffer);
    char **reply = get_words("Second call\n", ' ');
    printf("reply[0] = %s\n", reply[0]);
}

Stack Overflow is bursting with these questions. And local experts are not too eager to come to the rescue. This makes sense. For an experienced developer, sifting through a bunch of code in search of some boring mistake is not much fun. These mistakes usually come from gaps in programming language knowledge. So all the help the beginners usually get is a recommendation to read a certain chapter in a programming book — or to study documentation.

This doesn't mean experts are unwilling to help or are disrespectful. They are just not too excited about doing schoolwork-like tasks.

But let's go back to the Stack Overflow question I mentioned earlier. The question was already a couple of days old and still with no answer. How can one move forward from there?

Here's where a static analyzer can come to the rescue! Did you know that this tool can be useful both to experts and beginners? A static analyzer is a tool that performs a code review and reports suspicious code fragments. Static analyzers cannot replace a code review done by a teammate — but can be a great addition to the code review process since it helps find errors early.

So let's go ahead and run the PVS-Studio analyzer's online version for the code posted in the discussion. The first interesting and valuable warning that we get is the following: V1031 The 'malloc' function is not declared. Passing data to or from this function can be affected.

Since the malloc function has never been declared, it's unclear how the execution flow will behave. In the C programming language, if a function is used without having been declared first, this function is assumed to return int. However, in this case, the function returns a pointer. I've dedicated a note to why this is unsafe: "A nice 64-bit error in C". Let's fix this problem by adding #include <stdlib.h>.

Now the analyzer's output changes and we see another serious problem: 43:1: note: V773 The 'buffer' pointer was assigned values twice without releasing the memory. A memory leak is possible.

The error is here:

char *buffer = malloc(sizeof(char) * 100);
buffer = "hello world !\n";
....
free (buffer);

The pointer's value is wiped. To copy a string to the buffer correctly, one needs to use special functions, for example, strcpy. Let's fix the code.

The correct code:

#include <unistd.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <assert.h>
#include <stdlib.h>

char **get_words(char *buffer, char delimiter)
{
    printf("buffer = %s\n", buffer);
    char **words = malloc(sizeof(char *) * 100);
    if (words == NULL) {
        printf("Malloc Error\n");
        exit(84);
    }
    for (int i = 0; i < 100; i++) {
        words[i] = malloc(sizeof(char) * 100);
        if (words[i] == NULL) {
            printf("Malloc Error\n");
            exit(84);
        }
    }
    int word_count = 0;
    int l = 0;
    for (int i = 0; buffer[i] != '\0' && buffer[i]  != '\n'; i++, l++) {
        if (buffer[i] == delimiter) {
            words[word_count][l] = '\0';
            word_count++;
            l = -1;
        }
        else
            words[word_count][l] = buffer[i];
    }
    words[word_count][l] = '\0';
    return (words);
}

int main()
{
    char *buffer = malloc(sizeof(char) * 100);
    if (buffer == NULL)
        exit(84);
    strcpy(buffer, "hello world !\n");
    char **words = get_words(buffer, ' ');
    printf("words[0]= %s\n", words[0]);
    free (buffer);
    char **reply = get_words("Second call\n", ' ');
    printf("reply[0] = %s\n", reply[0]);
}

Although the corrected code is neither beautiful, nor safe — it now works fine. So the approach I've shown above can be a good way to find errors in code and to get help in the learning process.

The additional resources:

#Cpp