1

I have a struct country and an array of struct. I realloc but there is a problem in the members of the struct i can not understand. Here is the code

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


struct country{
    char* name;
    char* col;
    int num;
    char** nb;
    struct country* add;
};


int main(void){

    int ch, i = 0;
    struct country* A = (struct country*)malloc(sizeof(struct country));

    while(1){

         /*allocation for colour, name, */
        A[i].col = (char*)malloc(16*sizeof(char)); 
        A[i].name = (char*)malloc(16*sizeof(char));  /* BBBBUUUGGG HEEEERRRRREEEE  <<<<<<<<<<<<<<<<_________-----------------____________________---------------------*/ 

        A[i].nb = (char**)malloc(sizeof(char*));
        A[i].nb[0] = (char*)malloc(sizeof(char));

        A[i].num = 0;       /* define number of countries and then increase it*/
        scanf("%s %s", A[i].col, A[i].name);         /*value for colour, name of current country(node)*/


        ch = getchar(); /* see if there are neighbors or next country or the end*/

        while((ch=='\t') || (ch==' ')){         /* whitespace or tab means neighbor */

            A[i].nb = (char**)realloc(A[i].nb, (A[i].num+1)*sizeof(char*));     /* increase the elements of array(names of neighbors) by one */
            A[i].nb[A[i].num] = (char*)malloc(16*sizeof(char));     /* allocate memory for the name of the next neighbor(he is the num'th neighbor)*/
            scanf("%s", A[i].nb[A[i].num]);
            (A[i].num)++;
            ch = getchar();

        }

        (A[i].num)--;       /*we have increased the number of neighbors one time more than the actal size */

        if(ch!=EOF){        /* means that we have another line and so another country */

            A = (struct country*)realloc(A, (i+1)*sizeof(struct country));  /* another country-->A should be bigger */
            i++;                

        }else{
            break;      /* END OF FILE no more countries*/
        }

    }



}


name==name of the country, col= colour of the county, num = number of neighbors of the country and **nb is an array with their names, ignore struct countr* add

S.S. Anne
  • 15,171
  • 8
  • 38
  • 76

2 Answers2

1

You first need to understand that there is no Array present in the entirety of the code you have posted. Your struct and its members are pointers to blocks of memory you must allocate and manage.

Your logic for your while (ch ...) loop is jumbled leading confusing expressions like:

   (A[i].num)--;       /* we have increased the number of neighbors one time more */
                       /* than the actal size */

And then multiple attempted reallocations without a single allocation/reallocation validation or any validation of input.

There is no need to cast the return of malloc, it is unnecessary. See: Do I cast the result of malloc?. When setting the size for your allocation, if you use the dereferenced type, you will always get your type size correct.

    ...
    while (1) {

        /*allocation for colour, name, */
        A[i].col = malloc (16 * sizeof *A[i].col);
        A[i].name = malloc (16 * sizeof *A[i].name);
        A[i].nb = malloc (sizeof *A[i].nb);
        A[i].nb[A->num] = malloc (sizeof *A[i].nb[A[i].num]);

        /* validate EVERY allocation */
        if (!A[i].col || !A[i].name || !A[i].nb || !A[i].nb[A->num]) { 
            perror ("malloc-col,name,nb");
            return 1;
        }

        /* validate EVERY input - protect array bounds */
        if (scanf ("%15s %15s", A[i].col, A[i].name) != 2) {
            fputs ("error: failed to read col & name.\n", stderr);
            return 1;
        }
        ...

(note: you must validate EVERY allocation and EVERY reallocation, just as you must validate EVERY input)

Note the use of the field-width modifier to limit the read of character to 15 saving +1 for the nul-terminating character in

        /* validate EVERY input - protect array bounds */
        if (scanf ("%15s %15s", A[i].col, A[i].name) != 2) {
            fputs ("error: failed to read col & name.\n", stderr);
            return 1;
        }

Further, you ALWAYS realloc using a temporary pointer. When realloc fails, it returns NULL and if fail to use a temporary pointer, you have just overwritten your pointer to your original block of memory with NULL creating a memory-leak because you have lost your reference to the original block which now cannot be freed. Instead, you do something like:

            /* ALWAYS realloc with a temprary pointer */
            void *tmp = realloc (A[i].nb, (A[i].num + 1) * sizeof *A[i].nb);
            if (!tmp) {     /* validate EVERY reallocation */
                perror ("realloc-A[i].nb");
                return 1;
            }
            A[i].nb = tmp;    /* assign new block to A[i].nb after validation */

For your control of your loop checking for '\t' and ' ', you can make the logic simpler by incorporating your EOF check in the loop itself and then realloc A after, e.g.

        /* see if there are neighbors or next country or the end*/
        while ((ch = getchar()) != EOF && (ch == '\t' || ch == ' ')) {
            if (scanf("%15s", A[i].nb[A[i].num]) != 1) {
                fputs ("error: EOF of read of A[i].nb[A[i].num].\n", stderr);
                return 1;
            }
            A[i].num++;

            /* ALWAYS realloc with a temporary pointer */
            void *tmp = realloc (A[i].nb, (A[i].num + 1) * sizeof *A[i].nb);
            if (!tmp) {     /* validate EVERY reallocation */
                perror ("realloc-A[i].nb");
                return 1;
            }
            A[i].nb = tmp;    /* assign new block to A[i].nb after validation */
            A[i].nb[A[i].num] = malloc (16 * sizeof *A[i].nb[A[i].num]);
            if (!A[i].nb[A[i].num]) {
                perror ("malloc-A[i].nb[A[i].num]");
                return 1;
            }
        }
        /* realloc A */
        void *tmp = realloc (A, (i + 1) * sizeof *A);
        if (!tmp) {     /* validate */
            perror ("realloc-A");
            return 1;
        }
        A = tmp;    /* assign reallocated block */
        i++;        /* increment counter ('ncountry' better choice than i?) */

After going back through your example, my best guess at what you are trying to accomplish is contained in the following example. If you provide a bit of sample input I can validate further.

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

struct country {
    char *name;     /* generally the '*' goes with the variable not the type */
    char *col;      /* char* a, b, c;  does NOT create 3 pointers to char... */
    int num;        /* char *a, b, c;  makes that clear */
    char **nb;
    struct country *add;
};

int main (void) {

    int ch, i = 0;
    struct country *A = malloc (sizeof *A);     /* allocate 1 struct */

    if (!A) {   /* validate EVERY allocation */
        perror ("malloc-A");
        return 1;
    }
    A[i].num = 0;

    while (1) {

        /*allocation for colour, name, */
        A[i].col = malloc (16 * sizeof *A[i].col);
        A[i].name = malloc (16 * sizeof *A[i].name);
        A[i].nb = malloc (sizeof *A[i].nb);
        A[i].nb[A->num] = malloc (sizeof *A[i].nb[A[i].num]);

        /* validate EVERY allocation */
        if (!A[i].col || !A[i].name || !A[i].nb || !A[i].nb[A->num]) { 
            perror ("malloc-col,name,nb");
            return 1;
        }

        /* validate EVERY input - protect array bounds */
        if (scanf ("%15s %15s", A[i].col, A[i].name) != 2) {
            fputs ("error: failed to read col & name.\n", stderr);
            return 1;
        }

        /* see if there are neighbors or next country or the end*/
        while ((ch = getchar()) != EOF && (ch == '\t' || ch == ' ')) {
            if (scanf("%15s", A[i].nb[A[i].num]) != 1) {
                fputs ("error: EOF of read of A->nb[A->num].\n", stderr);
                return 1;
            }
            A[i].num++;

            /* ALWAYS realloc with a temprary pointer */
            void *tmp = realloc (A[i].nb, (A[i].num + 1) * sizeof *A[i].nb);
            if (!tmp) {     /* validate EVERY reallocation */
                perror ("realloc-A->nb");
                return 1;
            }
            A[i].nb = tmp;    /* assign new block to A->nb after validation */
            A[i].nb[A[i].num] = malloc (16 * sizeof *A[i].nb[A[i].num]);
            if (!A[i].nb[A[i].num]) {
                perror ("malloc-A[i].nb[A[i].num]");
                return 1;
            }
        }
        /* realloc A */
        void *tmp = realloc (A, (i + 1) * sizeof *A);
        if (!tmp) {     /* validate */
            perror ("realloc-A");
            return 1;
        }
        A = tmp;    /* assign reallocated block */
        i++;        /* increment counter ('ncountry' better choice than i?) */
    }
}

Look things over and let me know if you have further questions.

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • 1
    Happens --- `:)` – David C. Rankin Jan 02 '20 at 04:20
  • Thorough, as always; I was hoping that this answer was coming back from an edit ;) – ad absurdum Jan 02 '20 at 04:21
  • Yes, I was working though the rather "challenging" logic and skipped over the reallocation at the end leading to the confusion of how may `A`s were being handled -- which totally changed the answer. It took a while to "re-tool" after going down that rabbit trail `:)` – David C. Rankin Jan 02 '20 at 04:24
  • Tricky reading; I was looking at the `realloc`ed `A`s wrt your original answer just as you went to edit, so I suspected that you saw the same thing ;) – ad absurdum Jan 02 '20 at 04:26
  • Chuckling... I'm still picking though the block of `A`s recovery. – David C. Rankin Jan 02 '20 at 04:33
  • Unless I am missing something, I don't see validations for either `A[i].nb[A->num] = malloc(...` or `A[i].nb[A[i].num] = malloc(...` in the revised code. Also, should OP `free` memory allocations? – ad absurdum Jan 02 '20 at 04:47
  • You are probably right, I was bouncing between sectioning the New Years ham and cleaning up afterwards -- I'll run though again. (and I know why now, in the 1-A code that was done within the `while` loop and the remainder above was moved out of the loop to a different place `:)` – David C. Rankin Jan 02 '20 at 05:58
  • @David C. Rankin Thank you for the answer, there are no allocation/reallocation validations because i thought that the people who would try to answer my question should focus on the main problem. I appreciate you answers really. – average Joe Jan 02 '20 at 15:15
  • @David C. Rankin I have two questions.Why you used A->num since A refers only to the first element of the array, i think you could do that only if previously you had increase A like that: A++, but i am using index(i) so alwasy A->num refers to the number of Neighbors of the first Country. Secondly i think that you should check the other answer by Adrian Mole. Basically he says that i have to first increase -i- and then realloc A. He is right. That was my problem, but thanks you again for all the effort you put into understanding the logic behind my code and trying to help me. – average Joe Jan 02 '20 at 17:06
  • If there is still an `A->num`, then it is a leftover from the first cut I did before I found your realloc of `A` at the bottom. If you have a pointer, you use the `'->'` (arrow) operator to access members, but `"[..]"` also functions as a dereference, so when you declare a block of `A` as you do with `realloc`, then `A[i]` dereferences the pointer providing type `struct country` so the `'.'` (dot) operator is used for member access. Where you see `""error: EOF of read of A->nb[A->num].\n"`, that is just a string literal for the error message I missed changing. – David C. Rankin Jan 02 '20 at 22:28
0

One problem you have is how you are increasing the size of the A array (using the i variable)!

In this code:

        if(ch!=EOF){        /* means that we have another line and so another country */
            A = (struct country*)realloc(A, (i+1)*sizeof(struct country));  /* another country-->A should be bigger */
            i++;                
        }else{
            break;      /* END OF FILE no more countries*/
        }

you have an "off-by-one" error!

Thus, on the first run through the while loop, i will have a value of zero, so your realloc call will create a new buffer big enough for just one structure (i + 1) - but you already have a buffer for one such structure!

Solution: You need to put the i++ line before the realloc call!

EDIT: You have a very similar problem in your inner while loop with this line:

    A[i].nb = (char**)realloc(A[i].nb, (A[i].num+1)*sizeof(char*)); 

but here it's probably better to fix it using (A[i].num+2) as you are using the (correct) A[i].num values several times before you increment it.

Adrian Mole
  • 49,934
  • 160
  • 51
  • 83
  • 2
    I don't want to get into a fight over this, but why was my "feel free to accept" comment deleted? (Possibly, it also incurred the downvote!) The comment was based on one suggested by a moderator on [Meta](https://meta.stackoverflow.com/a/297598/10871073) - OP has not yet accepted any answers. – Adrian Mole Jan 02 '20 at 02:11
  • 1
    I don't see where OP has commented that you have solved their problem; it may be too soon to prompt for acceptance.... Not sure about the DV; it could be about vote begging, but I suspect it might be because there are many problems in OP code which are not addressed in your answer (it isn't even clear what problem OP is asking about). A minimal answer might get the code working, but a complete answer would offer some helpful code review. – ad absurdum Jan 02 '20 at 02:30
  • 3
    @exnihilo OP posted a comment saying "Thanks, I hadn't spotted that!" (or something similar) - but this has also been deleted (possibly by OP themselves - in which case, I can maybe understand why my earlier comment was misunderstood). – Adrian Mole Jan 02 '20 at 02:35
  • 1
    @exnihilo But thanks for your reply: I can see now that my post is far from a complete answer, and that I was perhaps a bit hasty in "accept begging!" My edit *may* have added *some* value to the answer. – Adrian Mole Jan 02 '20 at 02:46