Problem with code allocating memory for struct

  • Follow


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;
}
0
Reply El_Durango (24) 12/4/2011 9:48:12 AM

Le 04/12/2011 10:48, Durango a écrit :
> #define BUFFER_SIZ 128

Hummm.

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

Humm. The answer int he struct is one char. This is probably a (commonly
used) trick to have a variable length array at the end of a structure.
The real size is most probably not 1.

Ie. the "anspacket" is probably just a string buffer of any size,
allocated with malloc(number_of_chars + sizeof(anspacket))

> int getQuestionPacket(struct questionPacket **qp);
> static int getQuestionCallback(void *fpArg, int argc, char **argv, char
> **azColName);

Yuk. Why a void* ? Use real types rather than having opaque types is
always better.

> int main()
> {
>    qpacket   *qp;
>    anspacket *ap;
> 	
>    int ret = -1;
> 	
>    /*test*/
>    createQuestionPacket((struct questionPacket **)&callbackArg);

A first advice: DO NOT USE CAST (especially pointers). Unless you
absolutely need them. And if you absolutely need them, ask yourself if
this is a good idea or not. (the only known legit case is unsigned char
vs. char due to historical POSIX oddities)

Here, you don't need any cast. &callbackArg is qpacket **.

>    /* allocate packet memory */
>    createQuestionPacket((struct questionPacket **)&qp);

createQuestionPacket should not allocate static-length buffers in
members. And why checking the first malloc return, if the other malloc
are unchecked ? :)

>    (*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);

Use strdup() rather than allocating predefined buffer and strcpy them.
Or the program is going to blow up if argv[*] is too long.

> void createAnswerPacket(struct answerPacket **ap)
> {
>    if((*ap=(struct answerPacket *)malloc(sizeof(anspacket)))==NULL)

(Here again a dirty cast). So you are allocating ONE char (which is
sufficient for the "" string). I'm wondering if this is going to be
sufficient for the answer :)

0
Reply xroche (185) 12/4/2011 11:05:03 AM


Xavier Roche , dans le message <jbfk4v$n34$1@news.httrack.net>, a
 �crit�:
> Yuk. Why a void* ? Use real types rather than having opaque types is
> always better.

Probably because of the prototype of sqlite3_exec.
0
Reply george54 (194) 12/4/2011 11:13:47 AM

Durango <El_Durango@yahoo.com> writes:

> 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);
> }

Don't multiply by sizeof(char), it is 1.

The fixed-size buffers you're allocating here could more easily be
defined as arrays.  That said, you'd be better off allocating them only
when you know how big they need to be.

> 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]);*/

I imagine one of the argv[] entries is missing or too long.

-- 
http://www.greenend.org.uk/rjk/
0
Reply rjk (492) 12/4/2011 11:14:38 AM

On Sun, 04 Dec 2011 11:13:47 +0000, Nicolas George wrote:

> Xavier Roche , dans le message <jbfk4v$n34$1@news.httrack.net>, a
>  écrit :
>> Yuk. Why a void* ? Use real types rather than having opaque types is
>> always better.
> 
> Probably because of the prototype of sqlite3_exec.

Yes, that's correct the api function sqlite3_exec I cannot do anything 
about the arguments it takes.
0
Reply El_Durango (24) 12/4/2011 11:20:56 AM

Durango <El_Durango@yahoo.com> writes:
> 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.

What this supposed to mean? Someone else ran it and died before he
could inform you about the results?
0
Reply rweikusat (2678) 12/4/2011 4:49:59 PM

Richard Kettlewell <rjk@greenend.org.uk> writes:
>Durango <El_Durango@yahoo.com> writes:
>
>> 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);
>> }
>
>Don't multiply by sizeof(char), it is 1.

So what's wrong with using sizeof(char)?   It is self-documenting,
correct, adds no run-time overhead, and helps supports architectures where
sizeof(char) != 1.

scott
0
Reply scott 12/4/2011 7:07:36 PM

Durango <El_Durango@yahoo.com> writes:

>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);
>}

did you not intend to allocate memory for choiceC and choiceD?

scott
0
Reply scott 12/4/2011 7:09:30 PM

Le 04/12/2011 20:07, Scott Lurndal a écrit :
> So what's wrong with using sizeof(char)?   It is self-documenting,
> correct, adds no run-time overhead, and helps supports architectures where
> sizeof(char) != 1.

A "typed" malloc (#define typed_malloc(TYPE,SIZE) (TYPE*)
malloc(sizeof(TYPE)*(SIZE))) might be clearer in such case.

[ BTW sizeof(char) is always 1.

<http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf>

[ sizeof ]
"When applied to an operand that has type char, unsigned char,or signed
char, (or a qualified version thereof) the result is 1."

 ]

0
Reply xroche (185) 12/4/2011 7:16:36 PM

On 2011-12-04, Scott Lurndal <scott@slp53.sl.home> wrote:
> Richard Kettlewell <rjk@greenend.org.uk> writes:
>>
>>Don't multiply by sizeof(char), it is 1.
>
> So what's wrong with using sizeof(char)?   It is self-documenting,
> correct, adds no run-time overhead, and helps supports architectures where
> sizeof(char) != 1.

There are no architectures where sizeof(char) != 1. The C standard
*defines* sizeof(char) to be 1.

Are you perhaps confused with the possibility that CHAR_BIT != 8? sizeof
does not give you the size in "bytes" or "octets" it gives you the size
in chars. So an architecture with CHAR_BIT == 32 where a single char is
32bits in length, still has sizeof(char) == 1.

-- 
John Tsiombikas
http://nuclear.mutantstargoat.com/
0
Reply nuclear6 (33) 12/4/2011 10:08:43 PM

John Tsiombikas , dans le message
<slrnjdnrrb.nuc.nuclear@goat.mutantstargoat.com>, a �crit�:
> Are you perhaps confused with the possibility that CHAR_BIT != 8? sizeof
> does not give you the size in "bytes" or "octets" it gives you the size
> in chars.

Actually, it does give the size in byte, but the byte it refers to is
precisely the unit of memory necessary to store a char.
0
Reply george54 (194) 12/4/2011 10:18:00 PM

10 Replies
39 Views

(page loaded in 0.166 seconds)

Similiar Articles:













7/25/2012 6:32:48 PM


Reply: