Beginner mistake regarding memory allocation

Question

beginner programmer here, I recently began learning the basics of c, I tried making a program that reads surnames and lastnames from a text document and stores them in structs and prints out the contents of the structs, however when executing I can't get any output, only random nonsense characters. What am I doing wrong?, I'm assuming that there is a problem with my usage of malloc() but I'm not sure.

The main file

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#define N 100

struct person {
    char surname[N];
    char lastname[N];
};


_Bool read_ord(char*surname, char*lastname, FILE* f){ 
    char *temp =(char*) malloc(1000);
    fgets(temp,N,f);

    surname = strtok(temp," ");
    lastname = strtok(NULL, " ");

    if(surname != NULL && lastname != NULL){
        return 1;
    }
    else
        return 0;

}



int main (){
    FILE *infil = fopen("personer.txt", "r");

    struct person p[10];
    char *surn;
    char *lastn;
    int a = 0;

    while(read_ord(surn, lastn, infil) == 1){
        strcpy(p[a].surname, surn);
        strcpy(p[a].lastname, lastn);
        printf("%s %s\n", p[a].surname, p[a].lastname);
        a++;
    }


}

personer.txt

Svensson Lars
Bolinder Annika
Kristersson Uffe
Andersson Lina
Stensdotter Erik

Welcome to SO. You forgot that in C all parameters are passed by making a copy that is used in the function. You are assigning to that copy: `surname = strtok(temp," ");` It will not be visible after leaving that function. As soon as you fix that, you will have a memory leak as you never free the memory after copying. — Gerhardh, May 12 '23 at 10:16
`char *temp =(char*) malloc(1000);`... Why? First of all, you don't read more than `100` characters into the memory; Secondly you never `free` the memory afterward leading to a memory leak. There's no need for dynamic allocation here to begin with, use a plain array. — Some programmer dude, May 12 '23 at 10:17
As for *part* of your problem, pass a pointer to the structure instead, and copy the string to the members instead. — Some programmer dude, May 12 '23 at 10:17

score 0 · Accepted Answer · answered May 12 '23 at 11:06

we dont need to do any memory allocation in the code you provided above.

the problem you were running into is that you are in read_ord taking in a char* that is a copy of the pointer value instead you should have taken in a char** as that would have been a copy of the pointers address.

You also forgot to call free for the temp variable. Creating a memory leak. Though memory leak in your project wasnt the reason the program wasnt working.

/* original */

_Bool read_ord(char*surname, char*lastname, FILE* f){ 
    char *temp =(char*) malloc(1000);
    fgets(temp,N,f);

    surname = strtok(temp," ");
    lastname = strtok(NULL, " ");

    if(surname != NULL && lastname != NULL){
        return 1;
    }
    else
        return 0;
}

/* kinda fixed still memory leak */

_Bool read_ord(char** surname, char** lastname, FILE* f){ 
    char *temp =(char*) malloc(1000);
    fgets(temp,N,f);

    *surname = strtok(temp," ");
    *lastname = strtok(NULL, " ");

    if(*surname != NULL && *lastname != NULL){
        return 1;
    }
    else
        return 0;
}

I took your whole program and rewrote it with bit comments and explanations and fixed some problems.

here is my version

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

/* include stdbool to get access to bool, true and false */
#include <stdbool.h>

#define N 100

struct person {
    char surname[N];
    char lastname[N];
};

bool read_ord(char* surname, char* lastname, FILE* fd)
{
    /* we know we that the one line can only be max 2N + 1 long plus null terminator */
    char tmp[2*N + 2];

    /* get line */
    fgets(tmp, 2*N + 1, fd);

    /* get the pointers to surname and lastname */
    char* tsrun = strtok(tmp, " ");
    char* tlastn = strtok(NULL, " ");

    /* check if both surname and lastname were found */
    if(tsrun != NULL && tlastn != NULL) 
    {
        /* copy from tmp to surname */
        strcpy(surname, tsrun);

        /* copy from tmp to lastname */
        strcpy(lastname, tlastn);
        
        return true;
    }

    return false;
}

int main()
{
    FILE* infile = fopen("personer.txt", "r");

    struct person p[10];
    int a = 0;

    /* 
    add check to see if a < 10 for buffer overflow
    also no need for temporary addresses for surname and lastname
    */
    while(read_ord(p[a].surname, p[a].lastname, infile) && a < 10)
    {
        printf("%s %s\n", p[a].surname, p[a].lastname);
        a++;
    }

    /* close file */
    fclose(infile);
    
    return 0;
}

Comments like: `// get line`, `// copy line ` and `// close file` are unnecessary and unhelpful. — Harith, May 12 '23 at 13:30
@Haris there is no ``` /* copy line */``` comment. The ``` copy from tmp to surname ``` comment indicates that we are copying from tmp buffer and not some newly allocated buffer. ``` // close file ``` is i think helpful as fclose was not used in the code give in the question thus explaining what fclose does is helpful. ``` /* get line */ ``` is only weak comment. Also i dont think adding comments is a bad practice and definitely not unhelpful. — Rixu-chan, May 12 '23 at 20:02

Beginner mistake regarding memory allocation

1 Answers1