problem with memory allocation and structs

Discussion in 'C Programming' started by Durango, Dec 4, 2011.

  1. Durango

    Durango Guest

    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;
    }
     
    Durango, Dec 4, 2011
    #1
    1. Advertising

  2. On Dec 4, 9:47 am, Durango <> wrote:

    > 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>
     
    Nick Keighley, Dec 4, 2011
    #2
    1. Advertising

  3. On 04 Dec 2011 09:47:29 GMT, Durango <> wrote:

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


    --
    Remove del for email
     
    Barry Schwarz, Dec 4, 2011
    #3
  4. Durango <> 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.


    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.

    <snip>
    > 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.

    > return 0;
    > }


    --
    Ben.
     
    Ben Bacarisse, Dec 4, 2011
    #4
  5. Durango

    Phil Carmody Guest

    Durango <> writes:
    > 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
    --
    Unix is simple. It just takes a genius to understand its simplicity
    -- Dennis Ritchie (1941-2011), Unix Co-Creator
     
    Phil Carmody, Dec 6, 2011
    #5
  6. Durango

    Durango Guest

    On Sun, 04 Dec 2011 21:30:17 +0000, Ben Bacarisse wrote:
    >
    >> 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!
     
    Durango, Dec 9, 2011
    #6
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Nafai
    Replies:
    7
    Views:
    324
    John Harrison
    Sep 29, 2004
  2. Roman Hartmann

    structs and dynamic memory allocation

    Roman Hartmann, Nov 4, 2003, in forum: C Programming
    Replies:
    11
    Views:
    521
    Roman Hartmann
    Nov 5, 2003
  3. Patricia  Van Hise

    structs with fields that are structs

    Patricia Van Hise, Apr 5, 2004, in forum: C Programming
    Replies:
    5
    Views:
    650
    Al Bowers
    Apr 5, 2004
  4. Ken
    Replies:
    24
    Views:
    3,896
    Ben Bacarisse
    Nov 30, 2006
  5. chris
    Replies:
    6
    Views:
    1,006
    chris
    Oct 28, 2005
Loading...

Share This Page