I have a 'Segmentation Problem' while printing parsed parts of a String

Question

I am writing a simple Shell for school assignment and stuck with a segmentation problem. Initially, my shell parses the user input to remove whitespaces and endofline character, and seperate the words inside the input line to store them in a char **args array. I can seperate the words and can print them without any problem, but when storing the words into a char **args array, and if argument number is greater than 1 and is odd, I get a segmentation error.

I know the problem is absurd, but I stuck with it. Please help me.

This is my parser code and the problem occurs in it: Screenshot

char **parseInput(char *input){
int idx = 0;
char **parsed = NULL;
int parsed_idx = 0;

while(input[idx]){
    if(input[idx] == '\n'){
        break;

    }
    else if(input[idx] == ' '){
        idx++;

    }
    else{
        char *word = (char*) malloc(sizeof(char*));
        int widx = 0; // Word index
        word[widx] = input[idx];
        idx++;
        widx++;
        while(input[idx] && input[idx] != '\n' && input[idx] != ' '){
            word = (char*)realloc(word, (widx+1)*sizeof(char*));
            word[widx] = input[idx];
            idx++;
            widx++;
        }

        word = (char*)realloc(word, (widx+1)*sizeof(char*));
        word[widx] = '\0';
        printf("Word[%d] --> %s\n", parsed_idx, word);

        if(parsed == NULL){
            parsed = (char**) malloc(sizeof(char**));
            parsed[parsed_idx] = word;
            parsed_idx++;
        }else{
            parsed = (char**) realloc(parsed, (parsed_idx+1)*sizeof(char**));
            parsed[parsed_idx] = word;
            parsed_idx++;
        }

    }
}
int i = 0;

while(parsed[i] != NULL){
    printf("Parsed[%d] --> %s\n", i, parsed[i]);
    i++;
}

return parsed;

}

The `malloc` function requires the size in bytes to allocate. For a dynamically allocated array that's usually the number of elements multiplied with the size of a single element. For a string the single element is a single `char`, which is `sizeof(char)` (which is always equal to `1`) not `sizeof(char *)` (which will typically be either `4` or `8` depending on platform). That makes `char *word = malloc(sizeof(char*));` wrong, it should be `char *word = malloc(sizeof(char));` (or just plain `char *word = malloc(1);`). — Some programmer dude, Apr 05 '20 at 18:53
A possible hint about your problem: When do you ever assign `NULL` to `parsed[parsed_idx]`? — Some programmer dude, Apr 05 '20 at 18:56
And lastly a definitive hint about your problem: Use a debugger to catch the crash as and when it happen, to find out where in your code it happens and what the values of all involved variables are at that point. — Some programmer dude, Apr 05 '20 at 18:57
I am suspicious of how you turned char **argv into char * . . . can you please show that. — Frank Merrow, Apr 05 '20 at 19:51
@Someprogrammerdude Thank you for your return, I've missed out the malloc problem. Problem still remains. I am using a debugger. It seems the problem is the allocation of 'parsed'. There is an unnecessarily created one more parsed object. I couldn't where it is happening in the code but I'm working on it. :) — Furkan Çetinkaya, Apr 05 '20 at 20:44
@FrankMerrow This is a shell implementation, I didn't get the arguments from terminal, but stdin with getline(). — Furkan Çetinkaya, Apr 05 '20 at 20:47

Some programmer dude · Accepted Answer · 2020-04-06T03:08:38.090

In your code you have the loop

while(parsed[i] != NULL) { ... }

The problem is that the code never sets any elements of parsed to be a NULL pointer.

That means the loop will go out of bounds, and you will have undefined behavior.

You need to explicitly set the last element of parsed to be a NULL pointer after you parsed the input:

while(input[idx]){
    // ...
}

parsed[parsed_idx] = NULL;

On another couple of notes:

Don't assign back to the same pointer you pass to realloc. If realloc fails it will return a NULL pointer, but not free the old memory. If you assign back to the pointer you will loose it and have a memory leak. You also need to be able to handle this case where realloc fails.
A loop like
```
int i = 0;

while (parsed[i] != NULL)
{
    // ...
    i++;
}
```
is almost exactly the same as
```
for (int i = 0; parsed[i] != NULL; i++)
{
    // ...
}
```
Please use a for loop instead, it's usually easier to read and follow. Also for a for loop the "index" variable (i in your code) will be in a separate scope, and not available outside of the loop. Tighter scope for variables leads to less possible problems.
In C you shouldn't really cast the result of malloc (or realloc) (or really any function returning void *). If you forget to #include <stdlib.h> it could lead to hard to diagnose problems.

score 1 · Answer 2 · answered Apr 06 '20 at 04:04

1

Also, a beginner might find the -pedantic switch helpful on your call to the compiler. That switch would have pointed up most of the other suggestions made here. I personally am also a fan of -Wall, though many find it annoying instead of helpful.

answered Apr 06 '20 at 04:04

Frank Merrow

949
1
8
19

Thanks for suggestion :) – Furkan Çetinkaya Apr 15 '20 at 22:29

I have a 'Segmentation Problem' while printing parsed parts of a String

2 Answers2