malloc problem

Code:

---

FILE *file_name(char *buf, char *msg, FILE *fp)
{
    printf(msg);
    if(gets(buf) == NULL) /* check for errors */
        error("Greska pri unosu, zavrsavam.\n", 1);
    strcat(buf, ".txt");
    if((fp = fopen(buf, "r")) == NULL) /* open file with student names */
        error("Ne mogu otvoriti navedeni file.\n", 2);
    return fp;
}

int number_of_lines(char *buf, FILE *fp)
{
    int qsts;
   
    qsts = 0;
    while(fgets(buf, MAXBUF, fp) != NULL)
        qsts++;
   
    fseek(fp, 0l, 0);
    return qsts;
}

void store_students(char *lineptr[], int students, char *buf, FILE *fp)
{
    int i;
   
    for(i = 0; i < students; i++) {
          fgets(buf, MAXBUF, fp);
          lineptr[i] = (char *) malloc(sizeof(strlen(buf) + 1));
          strcpy(lineptr[i], buf);
    }
    fseek(fp, 0l, 0);
}
int main(int argc, char *argv[])
{
    char buf[MAXBUF];  /* buffer */
    FILE *fp;          /* students file */
    FILE *predmet;    /* questions file*/
    int students;      /* number of students */
    int questions;    /* number of questions */
   
    fp        = file_name(buf, "Unesite razred: ", fp);
    predmet  = file_name(buf, "Predmet: ", predmet);
    students  = number_of_lines(buf, fp);
    questions = number_of_lines(buf, predmet);
   
    char **lineptr;
    lineptr = (char **) malloc(students);
    store_students(lineptr, students, buf, fp);
}

---

The store_students() function doesnt work for me.

I have no idea why. Am i mallocing wrongly?

How does it not work?

I get a dont send message when i run this program. I think it compiles but runing it it crashes.

The problem is in strcpy() function in store_students().

If i remove it, it works.

Did you #include <stdlib.h>?

Fixed it.

This was the problem:

lineptr[i] = (char *) malloc(sizeof(strlen(buf) + 1));

sizeof() :).

Gotta love ptrs! :D

That fixed it? It sounds unlikely, because sizeof(strlen(buf) + 1) is likely to be 2, 4 or 8. If buf has a length longer than the number of bytes in a size_t, you will get a buffer overflow.

> printf(msg);
> if(gets(buf) == NULL) /* check for errors */
The only useful thing to do with code like this is delete it.

Learning C would be a good idea as well.
Like
> lineptr[i] = (char *) malloc(sizeof(strlen(buf) + 1));
When to use strlen() and when to use sizeof()
Using them both together isn't what you want.

> lineptr = (char **) malloc(students);
Read the malloc FAQ
It will tell you how to allocate the right number of objects with the right size.


> The problem is in strcpy() function in store_students().
The problem is ALL your broken malloc calls.

Quote:

---

> printf(msg);
> if(gets(buf) == NULL) /* check for errors */
The only useful thing to do with code like this is delete it.

---

Whats wrong with that?
printf("%s", msg);
Sure.

Quote:

---

Learning C would be a good idea as well.

---

Isnt thats why im here? :/

I fixed all the mallocs.

Code:

---

char **lineptr;
lineptr = (char **) malloc(sizeof(char *) * students);

---

Code:

---

void store_students(char *lineptr[], int students, char *buf, FILE *fp)
{
    int i;
   
    for(i = 0; i < students; i++) {
          fgets(buf, MAXBUF, fp);
          lineptr[i] = (char *) malloc(strlen(buf) + 1);
          strcpy(lineptr[i], buf);
    }
    fseek(fp, 0l, 0);
}

---

The reason they are broken because i never really needed this type of malloc.

And K&R covers a little malloc, not much.

Quote:

---

Originally Posted by Tool

Whats wrong with that?

---

The use of gets(), which has no way of preventing a buffer overflow.

Quote:

---

Originally Posted by Tool

I fixed all the mallocs.

---

I suggest that you fix it like this:

Code:

---

char **lineptr;
lineptr = malloc(sizeof(*lineptr) * students);

---

So, instead of having to make sure that you got the sizeof correct, you get the compiler to make sure of that for you through sizeof(*lineptr).

Also if you are using standard C there is absolutely no need to cast your mallocs. This has been discussed over and over again on this forum, just search the archives or read the FAQ.

Im making this program for a teacher.

And im preety sure he's not gonna overflow it, thats why i used gets.

Im aware of all the benefits fgets has.

Quote:

---

So, instead of having to make sure that you got the sizeof correct, you get the compiler to make sure of that for you through sizeof(*lineptr).

---

Thanks for the advice!

Quote:

---

Originally Posted by Tool

Im making this program for a teacher.

And im preety sure he's not gonna overflow it, thats why i used gets.

---

I am pretty sure that many people who allowed such mistakes to creep into their code were pretty sure that those mistakes would never be exploited. I am also pretty sure that some of them were shamed on The Daily WTF when their mistakes were discovered, perhaps even by a user. Would you like to be next, pretty please? :)

> Im making this program for a teacher.
Surely not to pass onto other students!?

> And im preety sure he's not gonna overflow it, thats why i used gets.
And will you remember to do the right thing when you slip effortlessly from academia to the workplace?
Being 'right' is in the approach from the beginning, not what you can get away with for the moment.

If you have a style that has less bugs, you WILL spend less time debugging.

Quote:

---

Originally Posted by Tool

And K&R covers a little malloc, not much.

---

There is not that much to it -- malloc(x) where x is a number of bytes.

Code:

---

malloc(sizeof(strlen(buf) + 1));

---

is pretty unique :p You should look up the return type of a function if you are going to use it.

Using sizeof() on a string is a common mistake tho.

You're right Salem, i'll write the code the best i know and can - simply because then i'll get used to it and avoid errors later on.

Can you have a quick look at my code and tell me what did i miss/do wrong?

Code:

---

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

#define MAXBUF 100

void error(char *msg, int x);

void initrand();
int randint();

void menu();
void print_help();

FILE *file_name(char *buf, char *msg, FILE *fp)
{
    char *p;
    printf("%s", msg);
    if(fgets(buf, MAXBUF, stdin) == NULL) /* check for errors */
        error("Greska pri unosu, zavrsavam.\n", 1);
    if((p = strchr(buf, '\n')) != NULL)
        *p = '\0';
    strcat(buf, ".txt");
    if((fp = fopen(buf, "r")) == NULL) /* open file with student names */
        error("Ne mogu otvoriti navedeni file.\n", 2);
    return fp;
}

int number_of_lines(char *buf, FILE *fp)
{
    int lines;
   
    lines = 0;
    while(fgets(buf, MAXBUF, fp) != NULL)
        lines++;
   
    fseek(fp, 0l, 0);
    return lines;
}

void store_data(char *lineptr[], int students, char *buf, FILE *fp)
{
    int i;
   
    for(i = 0; i < students; i++) {
          if(fgets(buf, MAXBUF, fp) == NULL)
              error("reading failed.\n", 1);
          if((lineptr[i] = (char *) malloc(strlen(buf) + 1)) == NULL)
              error("malloc failed.\n", 2);
          strcpy(lineptr[i], buf);
    }
    fseek(fp, 0l, 0);
}

void print_students(int students, char *lineptr[])
{
    int i;
   
    for(i = 0; i < students; i++)
        printf("%d: %s", i + 1, lineptr[i]);
    printf("\nUkupno ucenika: %d\n", students);
   
}

#define YES 1
#define PROCEED 2
#define QUIT 3
int questioning(char *lineptr[], char *qsts[], int students, int num_qsts)
{
    int c;
    int option();                                /* get option wether to ask a student */
    int print_questions(int qsts, char *qsts[]);  /* prints questions */
   
    int rand;          /* random number */
    initrand();        /* initialize random seed */
   
    printf("\nZa slucajne brojeve, [ENTER].\n");
    printf("Za kraj,  pritisnite [x]\n\n");
    if((c = option()) == QUIT)
        return 'x';
    else if(c != EOF)
        c = '\n';
    while(c != EOF) {
        if(c != '\n' && c != 'x') { /* only a newline triggers a new rand student to be written to stdout */
            c = getchar();
            continue;
        }
        if(c == 'x')
            return 'x';
        printf("%d: ", rand = randint() % students + 1);
        printf("%s", lineptr[rand - 1]);
        printf("Pitati? 'd' za da, u suprotnom enter.\n");
        switch(option()) { /* ask the student with random questions? */
            case YES:
                if(print_questions(num_qsts, qsts) == 'x')
                    c = 0;
                else
                    return EOF;
                break;
            case QUIT:
                c = 'x';
                break;
            case EOF:
                return EOF;
            default:
                    break;
        }
    }
    return EOF;
}

int option()
{
    char buf[MAXBUF];
   
    if(fgets(buf, MAXBUF, stdin) == NULL)
        return EOF;
       
    if(buf[0] == 'd' && buf[1] == '\n')
        return YES;
    else if(buf[0] == 'x' && buf[1] == '\n')
        return QUIT;
    else
        return PROCEED;
}

int get_qstnumber(int questions)
{
    int num;
    char buf[MAXBUF];
   
    while(1) {
          if(fgets(buf, MAXBUF, stdin) == NULL)
              return EOF;
          num = atoi(buf);
          if(num > 1 && num < questions)
              return num;
          else
              printf("\ninterval od 1 do %d: ", questions);
    }
}
   
int print_questions(int questions, char *qsts[])
{
    int num;
    initrand();
   
    printf("Broj pitanja(u bazi %d pitanja): ", questions);
    if((num = get_qstnumber(questions)) == EOF)
        return EOF;
   
    int c, rand;
    while((c = getchar()) != EOF) {
        if(c != '\n' && c != 'x')
            continue;
        if(c == 'x')
            return 'x';
        printf("%d: ", rand = randint() % num + 1);
        printf("%s", qsts[rand - 1]);
    }
    return EOF;
}


#define UNSUPPORTED_VAL 4
void get_choice(char *buf, int *choice)
{
    if(fgets(buf, MAXBUF, stdin) == NULL) {
        *choice = EOF;
        return;
    }
    if(buf[0] == '1' && buf[1] == '\n')
        *choice = '1';
    else if(buf[0] == '2' && buf[1] == '\n')
        *choice = '2';
    else if(buf[0] == '3' && buf[1] == '\n')
        *choice = '3';
    else
        *choice = UNSUPPORTED_VAL;
}
int main(int argc, char *argv[])
{
    char buf[MAXBUF];  /* buffer */
    FILE *fp;          /* students file */
    FILE *predmet;    /* questions file*/
    int students;      /* number of students */
    int questions;    /* number of questions */
   
    fp        = file_name(buf, "Unesite razred: ", fp); /* open file containing student names */
    predmet  = file_name(buf, "Predmet: ", predmet);  /* open file containing questions */
    students  = number_of_lines(buf, fp);              /* count number of students (lines) */
    questions = number_of_lines(buf, predmet);          /* count number of questions (lines) */
   
    char **lineptr;
    char **qstsptr;
    lineptr = (char **) malloc(sizeof(char *) * students);
    qstsptr = (char **) malloc(sizeof(char *) * questions);
    store_data(lineptr, students, buf, fp);      /* store student names into lineptr */
    store_data(qstsptr, questions, buf, predmet); /* store questions into qstsptr */
   
    printf("\n[%d] ucenika nadjeno.\n", students);
    /**********************MAIN MENU******************************************/
    int choice;      /* menu choice */         
    do {
        menu();      /* print menu to stdout */
        get_choice(buf, &choice);
        if(choice == '1')  /* list student names */
                print_students(students, lineptr);
        else if(choice == '2')  /* questioning */
                choice = questioning(lineptr, qstsptr, students, questions);
        else if(choice == '3')  /* print help */
                print_help();
    } while(choice != EOF);
    /*************************************************************************/
    return 0;
}

void initrand()
{
    srand((unsigned)(time(0)));
}

int randint()
{
    return rand();
}

void print_help()
{
    printf("-->POMOC<--\n");
    printf("--da bi dodali pristup drugim razredima, potrebno je napraviti sljedece:\n");
    printf("--u lokalnom direktoriju ovog .exea napravite .txt file.\n");
    printf("--renameate ga u odgovarajuce ime, npr 4a.txt ili 4f.txt.\n");
    printf("--u njega unesite imena ucenika tog razreda\n");
    printf("--vrlo je bitno da je po jednom retku samo jedno ime i prezime!\n");
    printf("--isto tako je vrlo bitno da su ucenici poredani kao u imeniku!\n");
    printf("--u suprotnom program nece raditi ocekivano.\n");
    printf("--kada ga kreirate odavde mu napisite ime za otvaranje kako bi ga koristili.\n");
    printf("--vrlo je bitno da fileove sa ekstenzinojim 4c_pitani.txt i ostale ne DIRATE.\n");
    printf("--izbjegavati palatale i ostale hrvatske sumnike.\n\n");
}

void menu()
{
    printf("Glavni meni:\n");
    printf("      1 - lista svih ucenika ovog razreda.\n");
    printf("      2 - ispitivanje.\n");
    printf("      3 - pomoc.\n");
    printf("Izbor: ");
}

void error(char *msg, int x)
{
    printf(msg);
    getchar();
    exit(x);
}

---

> lineptr = (char **) malloc(sizeof(char *) * students);
You've already included stdlib.h, so remove the cast.
If you still get a warning, then stop compiling your C program with a C++ compiler.

But that might be tricky, since you're declaring a lot of variables in the middle of the code.
Now whilst this is valid in C99, finding a C99 compiler isn't that easy. GCC is pretty close; MS Visual studio will never be C99.


Also, use the style laserlight suggested, namely
p = malloc( num * sizeof(*p) );
It is a lot easier to remember, and will keep working even if the type of the pointer changes (one less edit to maintain).

> fseek(fp, 0l, 0);
Use the SEEK_SET constant (or whatever is appropriate here).


You're passing buf to a lot of functions, but it is only acting as a local variable.
Use
char buf[SIZE];
if you're NOT using it to actually return a result.

A good example usage is get_qstnumber()



> file_name()
You still risk buffer overflow here if the buffer is full, and then you try to strcat() a string onto the end of it.


> printf(msg);
Use fputs(msg), or print("%s",msg);
NEVER pass an unknown string as the format string to printf.
Any stray % characters will spell trouble.


> initrand();
This should really be called once at the start of main.
As it is, it is called inside a loop, which will have the effect of making it a lot less random.

Other than that, it's looking pretty good.
Compared to a lot of stuff posted here, the indentation is very understandable.

Ok, here it is.

Cant compile without casting malloc, will get a better compiler.

Code:

---

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

#define MAXBUF 100

void error(char *msg, int x);
void initrand();
int randint();
void menu();
void print_help();

char buf[MAXBUF];                    /* global buffer - used by the 4 below functions storing temp data */
FILE *file_name(char *msg, FILE *fp); /* prints the msg to stdout, opens a file depending on input name */
int number_of_lines(FILE *fp);        /* counts number of lines inside a file */
void store_data(char *lineptr[], int students, FILE *fp); /* stores the data to the array of pointers from the file fp*/
void get_choice(int *choice);        /* main menu choice */

void print_students(int students, char *lineptr[]); /* prints students to stdout */
int questioning(char *lineptr[], char *qsts[], int students, int num_qsts); /* function to ask students with random qsts */

int main(int argc, char *argv[])
{
    char buf[MAXBUF];  /* buffer */
    FILE *fp;          /* students file */
    FILE *predmet;    /* questions file*/
    int students;      /* number of students */
    int questions;    /* number of questions */
   
    fp        = file_name("Unesite razred: ", fp); /* open file containing student names */
    predmet  = file_name("Predmet: ", predmet);  /* open file containing questions */
    students  = number_of_lines(fp);              /* count number of students (lines) */
    questions = number_of_lines(predmet);          /* count number of questions (lines) */
   
    char **lineptr;
    char **qstsptr;
    lineptr = (char **) malloc(sizeof(*lineptr) * students);
    qstsptr = (char **) malloc(sizeof(*qstsptr) * questions);
    store_data(lineptr, students, fp);      /* store student names into lineptr */
    store_data(qstsptr, questions, predmet); /* store questions into qstsptr */
   
    printf("\n[%d] ucenika nadjeno.\n", students);
    initrand();
    /**********************MAIN MENU******************************************/
    int choice;      /* menu choice */         
    do {
        menu();      /* print menu to stdout */
        get_choice(&choice);
        if(choice == '1')  /* list student names */
                print_students(students, lineptr);
        else if(choice == '2')  /* questioning */
                choice = questioning(lineptr, qstsptr, students, questions);
        else if(choice == '3')  /* print help */
                print_help();
    } while(choice != EOF);
    /*************************************************************************/
    return 0;
}

FILE *file_name(char *msg, FILE *fp)
{
    char *p;
    printf("%s", msg);
    if(fgets(buf, MAXBUF - 4, stdin) == NULL) /* check for errors */
        error("Greska pri unosu, zavrsavam.\n", 1);
    if((p = strchr(buf, '\n')) != NULL)
        *p = '\0';
    strcat(buf, ".txt");
    if((fp = fopen(buf, "r")) == NULL) /* open file with student names */
        error("Ne mogu otvoriti navedeni file.\n", 2);
    return fp;
}

int number_of_lines(FILE *fp)
{
    int qsts;
   
    qsts = 0;
    while(fgets(buf, MAXBUF, fp) != NULL)
        qsts++;
   
    fseek(fp, 0l, SEEK_SET);
    return qsts;
}

void store_data(char *lineptr[], int students, FILE *fp)
{
    int i;
   
    for(i = 0; i < students; i++) {
          if(fgets(buf, MAXBUF, fp) == NULL)
              error("reading failed.\n", 1);
          if((lineptr[i] = (char *) malloc(strlen(buf) + 1)) == NULL)
              error("malloc failed.\n", 2);
          strcpy(lineptr[i], buf);
    }
    fseek(fp, 0l, SEEK_SET);
}

void print_students(int students, char *lineptr[])
{
    int i;
   
    for(i = 0; i < students; i++)
        printf("%d: %s", i + 1, lineptr[i]);
    printf("\nUkupno ucenika: %d\n", students);
   
}

#define YES 1
#define PROCEED 2
#define QUIT 3
int questioning(char *lineptr[], char *qsts[], int students, int num_qsts)
{
    int c;
    int option();                                /* get option wether to ask a student */
    int print_questions(int qsts, char *qsts[]);  /* prints questions */
    int rand;                                    /* random number */
   
    printf("\nZa slucajne brojeve, [ENTER].\n");
    printf("Za kraj,  pritisnite [x]\n\n");
    if((c = option()) == QUIT)
        return 'x';
    else if(c != EOF)
        c = '\n';
    while(c != EOF) {
        if(c != '\n' && c != 'x') { /* only a newline triggers a new rand student to be written to stdout */
            c = getchar();
            continue;
        }
        if(c == 'x')
            return 'x';
        printf("%d: ", rand = randint() % students + 1);
        printf("%s", lineptr[rand - 1]);
        printf("Pitati? 'd' za da, u suprotnom enter.\n");
        switch(option()) { /* ask the student with random questions? */
            case YES:
                if(print_questions(num_qsts, qsts) == 'x')
                    c = 0;
                else
                    return EOF;
                break;
            case QUIT:
                c = 'x';
                break;
            case EOF:
                return EOF;
            default:
                    break;
        }
    }
    return EOF;
}

int option()
{
    if(fgets(buf, MAXBUF, stdin) == NULL)
        return EOF;
       
    if(buf[0] == 'd' && buf[1] == '\n')
        return YES;
    else if(buf[0] == 'x' && buf[1] == '\n')
        return QUIT;
    else
        return PROCEED;
}

int get_qstnumber(int questions)
{
    int num;
   
    while(1) {
          if(fgets(buf, MAXBUF, stdin) == NULL)
              return EOF;
          num = atoi(buf);
          if(num > 1 && num < questions)
              return num;
          else
              printf("\ninterval od 1 do %d: ", questions);
    }
}
   
int print_questions(int questions, char *qsts[])
{
    int num;
    int get_qstnumber(int questions);
   
    printf("Broj pitanja(u bazi %d pitanja): ", questions);
    if((num = get_qstnumber(questions)) == EOF)
        return EOF;
   
    int c, rand;
    while((c = getchar()) != EOF) {
        if(c != '\n' && c != 'x')
            continue;
        if(c == 'x')
            return 'x';
        printf("%d: ", rand = randint() % num + 1);
        printf("%s", qsts[rand - 1]);
    }
    return EOF;
}


#define UNSUPPORTED_VAL 4
void get_choice(int *choice)
{
    if(fgets(buf, MAXBUF, stdin) == NULL) {
        *choice = EOF;
        return;
    }
    if(buf[0] == '1' && buf[1] == '\n')
        *choice = '1';
    else if(buf[0] == '2' && buf[1] == '\n')
        *choice = '2';
    else if(buf[0] == '3' && buf[1] == '\n')
        *choice = '3';
    else
        *choice = UNSUPPORTED_VAL;
}

void initrand()
{
    srand((unsigned)(time(0)));
}

int randint()
{
    return rand();
}

void print_help()
{
    printf("-->POMOC<--\n");
    printf("--da bi dodali pristup drugim razredima, potrebno je napraviti sljedece:\n");
    printf("--u lokalnom direktoriju ovog .exea napravite .txt file.\n");
    printf("--renameate ga u odgovarajuce ime, npr 4a.txt ili 4f.txt.\n");
    printf("--u njega unesite imena ucenika tog razreda\n");
    printf("--vrlo je bitno da je po jednom retku samo jedno ime i prezime!\n");
    printf("--isto tako je vrlo bitno da su ucenici poredani kao u imeniku!\n");
    printf("--u suprotnom program nece raditi ocekivano.\n");
    printf("--kada ga kreirate odavde mu napisite ime za otvaranje kako bi ga koristili.\n");
    printf("--vrlo je bitno da fileove sa ekstenzinojim 4c_pitani.txt i ostale ne DIRATE.\n");
    printf("--izbjegavati palatale i ostale hrvatske sumnike.\n\n");
}

void menu()
{
    printf("Glavni meni:\n");
    printf("      1 - lista svih ucenika ovog razreda.\n");
    printf("      2 - ispitivanje.\n");
    printf("      3 - pomoc.\n");
    printf("Izbor: ");
}

void error(char *msg, int x)
{
    printf("%s", msg);
    getchar();
    exit(x);
}

---

Hi, I would like to ask a question about a usage in your code.

In your store_data function you get a pointer from main and allocate memory inside the function. I also tried this method in my project but I couldn't access the data inside of the allocated memory in the function. How could you overcome this?

Thanks for any response.

Well you could always start your own thread and actually post your code. That's a good start.


Quzah.

> char buf[MAXBUF]; /* global buffer - used by the 4 below functions storing temp data */
No!
This goes into the 4 functions.

> Cant compile without casting malloc, will get a better compiler.
a) which compiler
b) what error messages
c) is you program called prog.c or prog.cpp (.cpp is bad)

If it's visual studio, then you will need to fix some of the inline variable declarations and move them to the start of a statement block.

Quote:

---

Originally Posted by Tool

Im making this program for a teacher.

And im preety sure he's not gonna overflow it, thats why i used gets.

---

Yeah. There's absolutely no reason to expect an instructor to check whether your program is actually correct.

The program is made for a clueluess teacher actually.

He's not a programmer, nor has anything to do with programming.

Thats why i was preety sure he wasnt gonna overflow it nor pass a file to its std input (checking for EOF).

Quote:

---

which compiler

---

DevC++ on Windows. Using this on Solaris with Netbeans works fine, but not on DevCpp.

Ofcourse its a .c file.

error message usually says cannot conver types.

Quote:

---

Originally Posted by mugen

Hi, I would like to ask a question about a usage in your code.

In your store_data function you get a pointer from main and allocate memory inside the function. I also tried this method in my project but I couldn't access the data inside of the allocated memory in the function. How could you overcome this?

Thanks for any response.

---

Dont know, it works for me.

Although if i allocate the pointers to pointers in the actual function itself, it does not work.

It might be a .c file, but you're still using a C++ compiler.

Code:

---

$ gcc foo.c
$ g++ foo.c
foo.c: In function ‘int main()’:
foo.c:5: error: invalid conversion from ‘void*’ to ‘char*’
$ cat foo.c
#include <stdlib.h>
#include <stdio.h>

int main ( ) {
  char *p = malloc(10);
  free(p);
  return 0;
}

---