2

I have to write a program with dynamic 2d array but I've got a problem. Everything works fine until I try to print the array with more than 4 rows. Up to 4 rows all looks good, but with 5th row the length of last row expands to 7631472 which cause a segmentation fault.

Edit:

Thank you guys for response, I did what you'd suggested me,. However I discovered that problem is somewhere else, it's in "lengths" array. Each time when I add mroe than these 4 items it starts to fill up with some random values from memory.

Code:

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

int addRow(int i, int **array2d, int * lengths) {
    int len, value;
    int counter;
    i++;
    scanf("%d", &len); /*write the length of each row*/
        *(array2d + i-1) = malloc(sizeof (int) * len);
        *(lengths + i-1) = len;
    for(counter = 0; counter < len; counter++) {
        scanf("%d", &value); /*write each element in row*/
        *(*(array2d+i-1)+counter) = value; 
    }
    return i;
}

void print(int *i, int **array2d, int * lengths) {
    int counter, counter2;
    for(counter = 0; counter < *i; counter++) {
        for(counter2 = 0; counter2 < *(lengths+counter); counter2++) {
            printf("%d ",*(*(array2d+counter)+counter2));
        }
        printf("\n");
    } 
}

void end(int *i, int **array2d, int * lengths, char * word) {
    int counter;
    for(counter = 0; counter < *i; counter++)
        free(*(array2d + counter));
    free(array2d);
    free(lengths);
    free(word);
}

int main() {
    int ** array2d = (int**) malloc (0*sizeof(int*));
    int * lengths = (int*) malloc (0*sizeof(int)); /*length of each row*/
    char * word = (char*) malloc (3*sizeof(char));
    int i, isEqual; /* i - number of rows */
    i = 0;

    while(1) {
        scanf("%s", word);
        isEqual = strcmp(word, "add");
        if(!isEqual)
        i = addRow(i, array2d, lengths);
        isEqual = strcmp(word, "print");
        if(!isEqual) 
            print(&i, array2d, lengths);
        isEqual = strcmp(word, "end");
        if(!isEqual) {
            end(&i, array2d, lengths, word);
            return 0;
        } 
    }
    return 0;
}
Community
  • 1
  • 1
Wojtek
  • 33
  • 5
  • Yes, your're write, I add c++ tag by mistake. – Wojtek Apr 09 '17 at 14:42
  • the posted code is missing the this statement: `#include ` for the `strcmp()` function. – user3629249 Apr 09 '17 at 15:24
  • for ease of readability and understanding: 1) please consistently indent the code. 2) follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* 3) separate code blocks (for, if, else, while, do...while, switch, case, default) via a single blank line. 4) separate functions by 2 or 3 blank lines (be consistent) – user3629249 Apr 09 '17 at 15:26
  • when calling any of the heap allocation functions (malloc, calloc, realloc), always check (!=NULL) the returned value to assure the operation was successful. – user3629249 Apr 09 '17 at 15:27
  • when calling any of the `scanf()` family of functions, always check the returned value (not the parameter values) to assure the operation was successful. – user3629249 Apr 09 '17 at 15:30
  • 1
    due to precedence of C operators and the fact that `array2d` is actually a pointer to a pointer, this kind of expression will not work correctly. `*(array2d + i-1)` suggest using: `*(*array2d + i-1)`. Similar considerations exist for `(*(array2d+counter)` should be: `(*(*array2d+counter)` and `(array2d + counter)` should be: `(*array2d + counter)` – user3629249 Apr 09 '17 at 15:34
  • the returned value from the heap allocation functions has type: `void*` which can be assigned to any pointer. Casting the returned value just clutters the code, making it more difficult to understand, debug, etc. BTW: the expression: `sizeof(char)` is defined in the standard as 1. Multiplying anything by 1 has no effect. Suggest removing that expression from the parameters to `malloc()` – user3629249 Apr 09 '17 at 15:39
  • this line: `*(lengths + i-1) = len;` is undefined behavior because no heap memory was actually allocated in this statement: `int * lengths = (int*) malloc (0*sizeof(int));` Similar considerations exist for: `*(array2d + i-1) = malloc(sizeof (int) * len);` because no 'array of pointers to char' was actually allocated with: `int ** array2d = (int**) malloc (0*sizeof(int*));` – user3629249 Apr 09 '17 at 15:46
  • this line: ` char * word = (char*) malloc (3*sizeof(char));` allocates 3 bytes.. Then this line: `scanf("%s", word);` allows the user to enter any number of characters. When the user enters more than 2 characters, the allocated memory is overrun. This results in undefined behavior and can lead to a seg fault event. When using the '%s' input/conversion specifier, always include a MAX CHARACTERS modifier that is one less than the length of the input buffer to avoid any buffer overflow – user3629249 Apr 09 '17 at 15:50
  • `int **array2d` is not a 2D array. It is a pointer to a pointer. A 2D array can not be allocated. `char x[3][5]` is an example of a 2D array. [`char (*y)[3][5]`](https://cdecl.org/?q=char+%28*y%29%5B3%5D%5B5%5D) is pointer to a 2D array and memory for the 2D array can be allocated. – chux - Reinstate Monica Apr 09 '17 at 18:49

2 Answers2

2

In the main, you allocate arrays of size 0, if we read the man of malloc:

The malloc() function allocates size bytes and returns a pointer to the allocated memory. The memory is not initialized. If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().

This mean you create an undefined behavior, so it can work on every cases, as it can never work.

Instead of this, you should allocate arrays of size 1, and then increase there sizes with realloc when you need it.

Phantom
  • 833
  • 1
  • 9
  • 26
0

You should allocate array2d first to allocate *array2d.

The second pointer ,*array2d, points to the first new row cell that will contains your typed data. But first, you have to allocate a new item of your typed data rows list:

array2d = (int**) realloc(array2d, i)

Try this:

int addRow(int i, int **array2d, int * lengths) {
    int len, value;
    int counter;
    i++;
    array2d = (int**) realloc(array2d, sizeof (int*) * i);  //<<== realloc adding a new list item
    scanf("%d", &len); /*write the length of each row*/
        *(array2d + i-1) = malloc(sizeof (int) * len);
        *(lengths + i-1) = len;
    for(counter = 0; counter < len; counter++) {
        scanf("%d", &value); /*write each element in row*/
        *(*(array2d+i-1)+counter) = value; 
    }
    return i;
}
Ciro Corvino
  • 2,038
  • 5
  • 20
  • 33
  • `realloc(array2d, i);` is allocating insufficient memory. – chux - Reinstate Monica Apr 09 '17 at 18:55
  • 1
    It _should_ be `array2d = realloc(array2d, sizeof *arry2d * i);` `i` by itself is too small and cast is still not needed. – chux - Reinstate Monica Apr 09 '17 at 19:16
  • It needn't to realloc the whole buffer every time , you may simply treat your double pointer as a list of pointers, so it is sufficient to realloc only the pointers list (array2d) allocating memory for a new int pointer, a new list item, incrementing the items counter to each step of addRow function. – Ciro Corvino Apr 09 '17 at 19:55
  • To cast is always a best and safe practice. Mostly if you think that an int type machine implementation may vary among several compilers and computers.. – Ciro Corvino Apr 09 '17 at 19:59
  • "so it is sufficient to realloc only the pointers list" is true, but look carefully at `realloc(array2d, i)`. If `i==1`, that allocates **1 byte**, not even enough for 1 pointer. – chux - Reinstate Monica Apr 09 '17 at 20:32
  • Concerning "To cast is always a best and safe practice", - your coding error was a good example of why not to cast. Many would disagree that it is good to cast. [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/2410359) – chux - Reinstate Monica Apr 09 '17 at 20:35
  • 1
    Notice in `malloc(sizeof (int) * len);`, code uses `sizeof (int) *` - why? (it is good that it does) and notice in `realloc(array2d, i);` does not use `sizeof (int*) *` - which is bad. – chux - Reinstate Monica Apr 09 '17 at 20:37
  • It is an error that rises very easily to run time, making a simple test you see it immediately.. not so easy a different int type implementation... – Ciro Corvino Apr 09 '17 at 20:38
  • 1
    Post is now functionally correct with your edit. Note: code can use `array2d = realloc(array2d, sizeof *arry2d * i);` to avoid mis-scaling the `i`. – chux - Reinstate Monica Apr 09 '17 at 20:53