Array of Char pointers (strings) crashes after input?

Question

I want to store an array of strings , count their length and re-arrange them with length-increasing-order (smallest->larger) using the algorithm mentioned below //
Swap holds a relatively big string to replace the order (when another min is found)
I could use realloc() but I am not considering defend programming yet

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



int main()
    {
        int i,j,N;
        printf("\n Input amount of alphanumericals: ");
        scanf("%d",&N);
            {
                int min;
                char *swap=(char*)malloc(sizeof(char)*150);
                char *A[N],**temp;
                temp=A;
                for(i=0;i<N;i++){
                    printf("\nInput %d element:",i+1); 
                    fgets(temp+i,150,STDIN);
                }


    printf("\n\nData [");
    for(i=0;i<N;i++)
        printf(" %s",A[i]);
    printf(" ]\n\n");

    //Ins sort
    for(i=0;i<N;i++){
        min=i;//Assume current is min
        for(j=i+1;j<N;j++){
            //Compare assuming min with current
            if(strcmp(A[j],A[min])<0){
                min=j;
            }
            //If next is min replace in current position
        if(min!=i){
            swap=A[i];
            A[i]=A[min];
            A[min]=swap;
        }   
    }
    free(swap);
    printf("\nAfter insertion point algorithm\n");
    printf("\n\nInsertion Sorted Data [");
    for(i=0;i<N;i++)
        printf(" %s",A[i]);
    printf(" ]");
    }
    return 0;
}

Answer 1

You are tying to free memory that has not been allocated using malloc:

char *A[N],**temp;
temp = A;   // A is an automatic (AKA "stack") variable; now temp points to it as well
free(temp); // Undefined behavior

Inside the loop you are reading with gets into strings that have not been allocated:

gets(temp+i); // That's the same as gets(A[i]); Your A[i]s are unallocated.

As a side note, you should not use gets, because it is a prime source of buffer overruns. Use fgets instead, and pass stdin as the FILE* parameter. scanf with %20s is another alternative to limit the size.

In addition, since i goes from 1 to N, inclusive, this expression references one element past the A array on the last iteration:

gets(temp+i); // Undefined behavior when i==N

EDIT : Why is the code below crashes?

for(i=0;i<N;i++){
    printf("\nInput %d element:",i+1); 
    fgets(temp+i,150,STDIN);
}

The code below crashes because you did not allocate memory for the individual strings:

for(i=0;i<N;i++){
    printf("\nInput %d element:",i+1);
    temp+i = malloc(151); // No need to multiply by sizeof(char) or cast to char*
    fgets(temp+i,150,STDIN);
}

Note that you need to allocate one extra char for the null terminator.

Answer 2

You are not using gets correctly: take a look at its manual page [1]. Moreover, using gets is not recommended: better use fgets.

Also, gets is not allocating the string for you, so when you pass it a char * pointer, the pointer has to point to a valid memory location. Instead, your A[n] is an array of dangling pointers.

Then, why free(temp)? You must call free for each pointer allocated with malloc: not the case for temp.

Finally, please format your code: use indent.

[1] http://linux.die.net/man/3/gets