Dynamic string matrix in C

Question

I am having the next problem. I'm trying to create a dynamic matrix of strings which dimensions are defined by the user. The next code I made returns a segmentation fault:

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

int main(){
    char ***pa = NULL;
    int rows, columns , max, i, j = 0;

    //Ask dimension
    puts("Type number of columns: ");
    scanf("%d",&columns);
    puts("Type number of rows: ");
    scanf("%d",&rows);
    puts("Type max string length: ");
    scanf("%d",&max);

    //Allocate in memory
    ***pa = (char ***)malloc(rows * sizeof(char**));
    for(i=0;i<rows; i++){
        pa[i] = (char **)malloc(columns * sizeof(char*));
        for(j=0; i<columns;j++){
            pa[i][j] = (char)malloc((max+1)*sizeof(char));
        }
    }
    puts("Memory OK");

    //Fill the matrix
    i = 0;
    j = 0;
    for(i=0;i<rows;i++){
        strncpy(pa[i][j] , "Hello world", max);
        for(j=0;j<columns;j++){
            strncpy(pa[i][j] , "Hello world", max); 
        }
    }

    //Shows the matrix
    for(i=0;i<rows;i++){
        puts(pa[i][j]);
        for(j=0;j<columns;j++){
            puts(pa[i][j]);
        }
    }
    //Cleans the matrix
    free(pa);

    return 0;
}

I know I should check if the returned pointer by malloc is NULL but I didn't included it yet. The program does not print this so the problem is in the malloc for's

puts("Memory OK");

How can I debug this so I could know what happend? Did I made a mistake while coding it?

`***pa = (char ***)malloc(rows * sizeof(char**));` should be `pa = malloc(rows * sizeof *pa);`. — mch, Nov 15 '16 at 14:16
Excuse me for the tag, I didnt't notice. I though every variable should be initialized, even pointers. Is it better to not set it to NULL? — Carlos Manrique Enguita, Nov 15 '16 at 14:17
Compile with warnings enabled and treat the warnings as errors. — Jabberwocky, Nov 15 '16 at 14:21
1) There is no matrix (aka 2D array) in your code, nothing which can point to one or represent one. 2) You apparently want a 3D array. Why don't you use one? 3) Being a 3-star C programmer is not a compliment. — too honest for this site, Nov 15 '16 at 14:42
I know that in fact is a 3D array. Im trying to do it in this way as a personal execise. Sure there are other ways but I want to learn how to do it like that. — Carlos Manrique Enguita, Nov 15 '16 at 14:56
You do not 0-terminate your arrays if `max` is smaller than `12` (`strlen("Hello world") + 1`. — mch, Nov 15 '16 at 16:31
@mch e.g. doing`pa[i][j][max+1] = "\0"` would null terminate the string?. Also, What problems can cause a not 0-terminate string? — Carlos Manrique Enguita, Nov 15 '16 at 18:21

StoryTeller - Unslander Monica · Answer 1 · 2016-11-15T16:22:59.690

I'd recommend you leverage c99 to the fullest, and allocate the entire matrix as a single block. You'll be reducing the fragmentation of your programs memory and making iteration over the matrix more cache friendly:

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

int main(void) {

    int rows, columns , max;

    puts("Type number of columns: ");
    scanf("%d",&columns);
    puts("Type number of rows: ");
    scanf("%d",&rows);
    puts("Type max string length: ");
    scanf("%d",&max);

    char (*mat)[columns][max+1] = malloc(rows * columns * (max + 1));

    for(int i = 0; i < rows; ++i){
        for(int j = 0; j < columns; ++j) {
            strncpy(mat[i][j] , "Hello world", max); 
        }
    }

    for(int i = 0; i < rows; ++i){
        for(int j = 0; j < columns; ++j){
            puts(mat[i][j]);
        }
    }

    free(mat);

    return 0;
}

Other added bonii:

Removes the complexity involved with allocating your memory. No more nested loops with mallocs
No need to loop for freeing the whole thing, just free the one pointer. You already do that, but in your case it leaks memory.

Cons:

May fail to allocate the entire block for large matrices. Here the fragmented approach may be better.

So In the way I am doing elements are not near each other in memory. Are they? — Carlos Manrique Enguita, Nov 15 '16 at 14:58

AhmadWabbi · Accepted Answer · 2016-11-15T15:22:32.123

0

Change

pa[i][j] = (char)malloc((max+1)*sizeof(char));

into:

pa[i][j] = (char *)malloc((max+1)*sizeof(char));

Also, no need for the outer (first) strncpy in the main.

[EDIT]

Also, the first allocation line ***pa = ... should be pa = .... You want to assign an address to the pointer pa, not de-reference it (***pa de-references the pointer pa that has not been yet initialized).

edited Nov 15 '16 at 15:22

answered Nov 15 '16 at 14:16

AhmadWabbi

2,253
1
20
35

2

[The `(char*)` cast shouldn't be there at all.](http://stackoverflow.com/q/605845/1679849) – r3mainer Nov 15 '16 at 14:20
@squeamishossifrage Why that? – AhmadWabbi Nov 15 '16 at 14:21
That was a link I posted. – r3mainer Nov 15 '16 at 14:23
@squeamish ossifrage Do not give references to wrong answers of low-qualified programmers. – Vlad from Moscow Nov 15 '16 at 14:23
@AhmadWabbi He is just wrong along with the bad answer in the given reference. – Vlad from Moscow Nov 15 '16 at 14:23
@VladfromMoscow Lol ok Vlad – AhmadWabbi Nov 15 '16 at 14:24
@AhmadWabbi IIf something is allowed it does not mean that it should be done.:) – Vlad from Moscow Nov 15 '16 at 14:26
3

@VladfromMoscow, given the immense amount of contribution [**unwind**](http://stackoverflow.com/users/28169/unwind) made to the `C` tag, calling him "low-qualified" is not only wrong, it's downright ludicrous. You may not agree with him, but your reason is a non-argument based on nothing but an ad-hominem attack. In fact, this question demonstrates exactly why casting when you don't have to is a good way to shoot yourself in the foot. – StoryTeller - Unslander Monica Nov 15 '16 at 14:53
@StoryTeller I am sure that low-qualified programmers is much more than qualified programmers. Thus you are saying about the immense amount of low-qualified programmers. And I agree with you. They are indeed too many.:) – Vlad from Moscow Nov 15 '16 at 15:03
@StoryTeller As for arguments then you are not the first who asks me. I already answered such question many times. I have not a desire to answer one thousand and one times. – Vlad from Moscow Nov 15 '16 at 15:11

score 0 · Answer 3 · answered Nov 15 '16 at 16:03

0

Thanks to veryone for helping me to solve it. I've learned new things with you. First of all, I changed this.

pa = malloc(rows * sizeof(char**));
for(i=0;i<rows; i++){
    pa[i] = malloc(columns * sizeof(char*));
    for(j=0; j<columns;j++){//Here there was an i. That was totally wrong.
        pa[i][j] = malloc((max+1)*sizeof(char));
    }
}

Now it works and allocates correctly the matrix into de memory.

answered Nov 15 '16 at 16:03

Carlos Manrique Enguita

33
1
8

So, you did what I suggested in my answer, but without the casting (even though you may have kept it). In SO, this is usually translated to an up-vote, as an expression of gratitude for those who spent time helping you. I am telling you this just because you are new here, no need to your up-vote anyway. – AhmadWabbi Nov 15 '16 at 21:58
As you say I just registered in the page, Im still learning how this works. Excuse me if I made a mistake. – Carlos Manrique Enguita Nov 15 '16 at 22:08
No problem, I knew that. I just wanted to draw your attention to it so that the others do not take it bad in the future. Good luck. – AhmadWabbi Nov 15 '16 at 22:31

Dynamic string matrix in C

3 Answers3