problem with memory allocation and structs

D

Durango

Hello I am working on a program that requires me to use structs. I am
allocating memory for the struct and the variables defined in them.
I am sure there is something incorrect with my code because there is
undefined behavior when running the program.

I am posting the code so if any of you can catch the problem.
I am new still to this so I am sure it's something trivial. I appreciate
any help.

Also are there any good guides/books that can get into more detailed
explanation of pointers and memory allocation so that I can have a more
in depth understanding?

Here is the code I am working with:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sqlite3.h>
#define BUFFER_SIZ 128

typedef struct questionPacket
{
char *id;
char *data;
char *choiceA;
char *choiceB;
char *choiceC;
char *choiceD;
}qpacket;

typedef struct answerPacket
{
char answer[1];
}anspacket;

void createQuestionPacket(struct questionPacket **qp);
void createAnswerPacket(struct answerPacket **ap);
void initializeSQLite();
int getQuestionPacket(struct questionPacket **qp);
static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName);

static sqlite3 *db;
static char *dbfile = "millionaire.db";
static int retval;
static char *zErrMsg = 0;
qpacket *callbackArg;

int main()
{
qpacket *qp;
anspacket *ap;

int ret = -1;

/*test*/
createQuestionPacket((struct questionPacket **)&callbackArg);
/* allocate packet memory */
createQuestionPacket((struct questionPacket **)&qp);
createAnswerPacket((struct answerPacket **)&ap);
initializeSQLite();
printf("after initializeSQLite\n");
ret = getQuestionPacket((struct questionPacket **)&qp);

printf("id is %s\n",callbackArg->id); /* id has correct
value */
printf("question is %s\n", callbackArg->data); /* data has incorrect
value */
return 0;
}


void createQuestionPacket(struct questionPacket **qp)
{
if((*qp=(struct questionPacket *)malloc(sizeof(qpacket)))==NULL)
{
fprintf(stderr, "No memory was allocated for change question packet!
\n");
exit(0);
}
(*qp)->id = malloc(sizeof(char) * 8);
(*qp)->data = malloc(sizeof(char) * 256);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceB = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
}


void createAnswerPacket(struct answerPacket **ap)
{
if((*ap=(struct answerPacket *)malloc(sizeof(anspacket)))==NULL)
{
fprintf(stderr, "No memory was allocated for double dip packet!\n");
exit(0);
}
}

void initializeSQLite()
{
/* connect to DB */
retval = sqlite3_open_v2(dbfile, &db, SQLITE_OPEN_READWRITE, NULL);
if( retval )
{
fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
sqlite3_close(db);
exit(1);
}
}

int getQuestionPacket(struct questionPacket **qp)
{
char *questionSqlStmt;
questionSqlStmt = malloc(BUFFER_SIZ);
strcpy(questionSqlStmt,"select
id,question,choice_a,choice_b,choice_c,"\
"choice_d from questionTbl ORDER BY "\
"RANDOM
() LIMIT 1;");
retval = sqlite3_exec(db, questionSqlStmt, getQuestionCallback, &qp,
&zErrMsg);

return retval;
}

static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName)
{
callbackArg = (struct questionPacket *)fpArg;
strcpy(callbackArg->id,argv[0]);
strcpy(callbackArg->data,argv[1]);
/* uncommenting line below causes segmentation fault */
/*strcpy(callbackArg->choiceA,argv[2]);
strcpy(callbackArg->choiceB,argv[3]);
strcpy(callbackArg->choiceC,argv[4]);
strcpy(callbackArg->choiceD,argv[5]);*/

return 0;
}
 
N

Nick Keighley

Hello I am working on a program that requires me to use structs.  I am
allocating memory for the struct and the variables defined in them.
I am sure there is something incorrect with my code because there is
undefined behavior when running the program.

not really. You program may exhibit undefined behaviour but no
implementaion to my knowledge actually reports it as such. You are
more likely getting a segmentaion violoation or some such. You need to
explain exactlywhat the error is and where abouts it is occurring (use
printfs or a debugger)

The code you have posted is rather long could you post a short,
compilable example that exhibits your problem. You never know about
cutting down the problem to the bare minimum you may find the problem
yourself.

You might also think about trying some simpler problems to start with.
I am posting the code so if any of you can catch the problem.
I am new still to this so I am sure it's something trivial.  I appreciate
any help.

Also are there any good guides/books that can get into more detailed
explanation of pointers and memory allocation so that I can have a more
in depth understanding?

K&R isn't bad.

<snip>
 
B

Barry Schwarz

Hello I am working on a program that requires me to use structs. I am
allocating memory for the struct and the variables defined in them.
I am sure there is something incorrect with my code because there is
undefined behavior when running the program.

I am posting the code so if any of you can catch the problem.
I am new still to this so I am sure it's something trivial. I appreciate
any help.

Also are there any good guides/books that can get into more detailed
explanation of pointers and memory allocation so that I can have a more
in depth understanding?

Here is the code I am working with:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sqlite3.h>
#define BUFFER_SIZ 128

typedef struct questionPacket
{
char *id;
char *data;
char *choiceA;
char *choiceB;
char *choiceC;
char *choiceD;
}qpacket;

typedef struct answerPacket
{
char answer[1];
}anspacket;

void createQuestionPacket(struct questionPacket **qp);
void createAnswerPacket(struct answerPacket **ap);
void initializeSQLite();
int getQuestionPacket(struct questionPacket **qp);
static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName);

static sqlite3 *db;
static char *dbfile = "millionaire.db";
static int retval;
static char *zErrMsg = 0;
qpacket *callbackArg;

int main()
{
qpacket *qp;
anspacket *ap;

int ret = -1;

/*test*/
createQuestionPacket((struct questionPacket **)&callbackArg);

The cast serves no purpose in of these function calls.

This causes a memory leak (see the code in getQuestionCallback).
/* allocate packet memory */
createQuestionPacket((struct questionPacket **)&qp);

Do you really need two instances of this structure.
createAnswerPacket((struct answerPacket **)&ap);

Did you really want ap to point to an area capable of holding a single
byte?
initializeSQLite();
printf("after initializeSQLite\n");
ret = getQuestionPacket((struct questionPacket **)&qp);

printf("id is %s\n",callbackArg->id); /* id has correct
value */
printf("question is %s\n", callbackArg->data); /* data has incorrect
value */
return 0;
}


void createQuestionPacket(struct questionPacket **qp)
{
if((*qp=(struct questionPacket *)malloc(sizeof(qpacket)))==NULL)

You don't need to cast the value returned by malloc.
{
fprintf(stderr, "No memory was allocated for change question packet!
\n");
exit(0);
}
(*qp)->id = malloc(sizeof(char) * 8);

Are you aware that sizeof(char) is guaranteed to be 1?
(*qp)->data = malloc(sizeof(char) * 256);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceB = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
}


void createAnswerPacket(struct answerPacket **ap)
{
if((*ap=(struct answerPacket *)malloc(sizeof(anspacket)))==NULL)
{
fprintf(stderr, "No memory was allocated for double dip packet!\n");
exit(0);
}
}

void initializeSQLite()
{
/* connect to DB */
retval = sqlite3_open_v2(dbfile, &db, SQLITE_OPEN_READWRITE, NULL);
if( retval )
{
fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
sqlite3_close(db);

If you cannot open the database, does it make sense to try to close
it?
exit(1);
}
}

int getQuestionPacket(struct questionPacket **qp)
{
char *questionSqlStmt;
questionSqlStmt = malloc(BUFFER_SIZ);
strcpy(questionSqlStmt,"select
id,question,choice_a,choice_b,choice_c,"\
"choice_d from questionTbl ORDER BY "\
"RANDOM
() LIMIT 1;");
retval = sqlite3_exec(db, questionSqlStmt, getQuestionCallback, &qp,
&zErrMsg);

Unless sqlite3_exec frees the memory pointed to by questionSqlStmt
(something I hope is not true), you have a memory leak here.
return retval;
}

static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName)
{
callbackArg = (struct questionPacket *)fpArg;

callbackArg pointed to allocated memory. Now it points to different
allocated memory. The address of the first allocation is now
completely and irretrievably lost.
strcpy(callbackArg->id,argv[0]);

Maybe you would like to tell us what sqlite3_exec passes to this
function.
strcpy(callbackArg->data,argv[1]);
/* uncommenting line below causes segmentation fault */

I suggest that, for testing purposes, instead of copying the data you
simply print it and verify that all the strings are short enough to
fit in the allocated areas, particularly argv[0].
/*strcpy(callbackArg->choiceA,argv[2]);
strcpy(callbackArg->choiceB,argv[3]);
strcpy(callbackArg->choiceC,argv[4]);
strcpy(callbackArg->choiceD,argv[5]);*/

return 0;
}
 
B

Ben Bacarisse

Durango said:
Hello I am working on a program that requires me to use structs. I am
allocating memory for the struct and the variables defined in them.
I am sure there is something incorrect with my code because there is
undefined behavior when running the program.

I think you mean you get a run-time error, though the behaviour of your
program is, indeed undefined.
I am posting the code so if any of you can catch the problem.
I am new still to this so I am sure it's something trivial. I appreciate
any help.

There are quite a few things that I want to comment on about this
program (the use of far too many globals being the first!) but let me
make up for that by pointing to the source of your run-time fault.

(1)
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceB = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);

Stare at the last two of these until you see something wrong.

(2)
retval = sqlite3_exec(db, questionSqlStmt, getQuestionCallback, &qp,
&zErrMsg);

The third argument is wrong. qp isof type "struct questionPacket **" so
&qp is of type "struct questionPacket ***" but the callback function
expects it's first argument to be a "struct questionPacket *". You need

retval = sqlite3_exec(db, questionSqlStmt, getQuestionCallback, *qp,
&zErrMsg);

here instead. If this is not clear, ask why.

Here is the code I am working with:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sqlite3.h>
#define BUFFER_SIZ 128

typedef struct questionPacket
{
char *id;
char *data;
char *choiceA;
char *choiceB;
char *choiceC;
char *choiceD;
}qpacket;

At some point an array gets easier to use. I'd probably write

char *choice[4];
typedef struct answerPacket
{
char answer[1];
}anspacket;

void createQuestionPacket(struct questionPacket **qp);
void createAnswerPacket(struct answerPacket **ap);
void initializeSQLite();

void initializeSQLite(void);

is better (in C). Your version will not prevent the compiler from
reporting some invalid calls like initializeSQLite(42).
int getQuestionPacket(struct questionPacket **qp);
static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName);

static sqlite3 *db;
static char *dbfile = "millionaire.db";
static int retval;
static char *zErrMsg = 0;
qpacket *callbackArg;

Very few of these are needed (for example, look at how you use retval)
but even for those that seem to be needed you'd be better off not
sharing data between functions like this.
int main()

int main(void) as above (though the scope for errors is very much more
limited).
{
qpacket *qp;
anspacket *ap;

int ret = -1;

/*test*/
createQuestionPacket((struct questionPacket **)&callbackArg);
/* allocate packet memory */
createQuestionPacket((struct questionPacket **)&qp);
createAnswerPacket((struct answerPacket **)&ap);
initializeSQLite();
printf("after initializeSQLite\n");
ret = getQuestionPacket((struct questionPacket **)&qp);

printf("id is %s\n",callbackArg->id); /* id has correct
value */
printf("question is %s\n", callbackArg->data); /* data has incorrect
value */
return 0;
}

I don't think any of the casts you have in this whole program are
needed. They clutter the code and do nothing but hide possible errors.
void createQuestionPacket(struct questionPacket **qp)

As a general point, I never waste a return value. I.e. I very rarely
have void functions. I'd use the return for the pointer so that I would
no longer use a ** parameter. As a result, the prototype would be

struct questionPacket *createQuestionPacket(void);
{
if((*qp=(struct questionPacket *)malloc(sizeof(qpacket)))==NULL)

I'd write:

if ((*qp = malloc(sizeo **qp)) == NULL)

No cast and no need to repeat the type name.
{
fprintf(stderr, "No memory was allocated for change question packet!
\n");
exit(0);
}
(*qp)->id = malloc(sizeof(char) * 8);
(*qp)->data = malloc(sizeof(char) * 256);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceB = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
}

sizeof(char) is always 1 and there are too many mysterious sizes here.
You need to name the size because, later, you should limit the copy so
as not to run off the end of these allocated arrays.
void createAnswerPacket(struct answerPacket **ap)

Ditto about the return type.
{
if((*ap=(struct answerPacket *)malloc(sizeof(anspacket)))==NULL)

Ditto about the malloc call.
{
fprintf(stderr, "No memory was allocated for double dip packet!\n");
exit(0);
}
}

void initializeSQLite()
{
/* connect to DB */
retval = sqlite3_open_v2(dbfile, &db, SQLITE_OPEN_READWRITE, NULL);
if( retval )
{
fprintf(stderr, "Can't open database: %s\n", sqlite3_errmsg(db));
sqlite3_close(db);
exit(1);
}
}

int getQuestionPacket(struct questionPacket **qp)

You got into the habit of passing a "struct questionPacket **" but you
don't need to do that here. This function does not want to change its
argument you can just pass a "struct questionPacket *".
{
char *questionSqlStmt;
questionSqlStmt = malloc(BUFFER_SIZ);

There is no need to do this. For one thing, if you malloc memory you
should always free it, and you don't ever free this string. I show you
an alternative below.
strcpy(questionSqlStmt,"select
id,question,choice_a,choice_b,choice_c,"\
"choice_d from questionTbl ORDER BY "\
"RANDOM

The "\"s don't do anything useful here. Adjacent string literals get
concatenated with or without them.
() LIMIT 1;");
retval = sqlite3_exec(db, questionSqlStmt, getQuestionCallback, &qp,
&zErrMsg);

return retval;
}

Here's how I'd write this function:

int getQuestionPacket(struct questionPacket *qp)
{
char *questionSqlStmt =
"select id, question, choice_a, choice_b, choice_c, choice_d "
"from questionTbl order by random () limit 1;";

return sqlite3_exec(db, questionSqlStmt, getQuestionCallback, qp, &zErrMsg);
}

Note that now you don't need *qp as the 4th argument.
static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName)
{
callbackArg = (struct questionPacket *)fpArg;
strcpy(callbackArg->id,argv[0]);
strcpy(callbackArg->data,argv[1]);
/* uncommenting line below causes segmentation fault */
/*strcpy(callbackArg->choiceA,argv[2]);
strcpy(callbackArg->choiceB,argv[3]);
strcpy(callbackArg->choiceC,argv[4]);
strcpy(callbackArg->choiceD,argv[5]);*/

These copy operations should be limited by the sizes of the malloced
arrays. Sadly, that's a fiddly thing to do since strncpy does not do
what you might think. DONT USE IT! Many systems have the non-standard
strlcpy function to do this job.
 
P

Phil Carmody

Durango said:
typedef struct questionPacket
{
char *id;
char *data;
char *choiceA;
char *choiceB;
char *choiceC;
char *choiceD;
}qpacket; ....
void createQuestionPacket(struct questionPacket **qp)
{
if((*qp=(struct questionPacket *)malloc(sizeof(qpacket)))==NULL)
{
fprintf(stderr, "No memory was allocated for change question packet!
\n");
exit(0);
}
(*qp)->id = malloc(sizeof(char) * 8);
(*qp)->data = malloc(sizeof(char) * 256);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceB = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);
(*qp)->choiceA = malloc(sizeof(char) * 128);

Copy-pasto, I'd bet.
Did you intend to initialise choiceC and choiceD instead?

Phil
 
D

Durango

static int getQuestionCallback(void *fpArg, int argc, char **argv, char
**azColName)
{
callbackArg = (struct questionPacket *)fpArg;
strcpy(callbackArg->id,argv[0]);
strcpy(callbackArg->data,argv[1]);
/* uncommenting line below causes segmentation fault */
/*strcpy(callbackArg->choiceA,argv[2]);
strcpy(callbackArg->choiceB,argv[3]);
strcpy(callbackArg->choiceC,argv[4]);
strcpy(callbackArg->choiceD,argv[5]);*/

These copy operations should be limited by the sizes of the malloced
arrays. Sadly, that's a fiddly thing to do since strncpy does not do
what you might think. DONT USE IT! Many systems have the non-standard
strlcpy function to do this job.
return 0;
}

Good Lord! I missed that! sorry I had been working on my project for a
while and sleep depravation does not help. Thank you for that!
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top