1

gcc 4.4.5 c89

I have a function called create_object where I allocate memory for a global structure. And I have a function called destroy_object where I check that the pointer is not null, then I free. Just incase I free memory that hasn't been allocated. However, I have tested this by making 2 consecutive calls to destroy_object. However, I get a stack dump on the second call. However, I am sure that it would not free as I have assigned the pointer to NULL. So it should skip the free function.

static struct Config_t {
    char protocol[LINE_SIZE];
    char mode[LINE_SIZE];
} *app_cfg = NULL;

int create_object()
{
    app_cfg = malloc(sizeof *app_cfg);
    memset(app_cfg, 0, sizeof *app_cfg);
}

void destroy_config()
{
    /* Check to see if the memory is ok to free */
    if(app_cfg != NULL) {
        free(app_cfg);
        app_cfg = NULL;
    }
}

Many thanks for any suggestions,

================= EDIT ========== Basicially I have in my main function a call to create_object() and I do some processing and then make a call to destory_object.

int main(void)
{
    create_object();

    /* Do some processing on the structure */

    destroy_object();

    return 0;
}

========================= Final Edit ==== static struct Config_t { char protocol[LINE_SIZE]; char mode[LINE_SIZE]; } app_cfg[1] {{"", ""}};

And now I am not using malloc and free.

ant2009
  • 27,094
  • 154
  • 411
  • 609
  • 1
    That looks fine to me, can you post the code that uses this pointer and makes calls to `create_object` and `destroy_config`? Also, if you want to immediately initialize your memory allocated to `app_cfg` to 0s, you can combined your `malloc` and `memset` calls into one `calloc` call. Also, `free` on a null pointer is perfectly fine. – wkl Nov 22 '10 at 05:46
  • 1
    Passing a null pointer to `free()` is a safe no-op, so you don't need the null check in `destroy_config()`. – Wyzard Nov 22 '10 at 05:49
  • 2
    are you able to run the code through gdb and look at the backtrace after the SIGSEGV is raised? Does it point to any other place in your program? – vpit3833 Nov 22 '10 at 06:10

2 Answers2

3

I have only one suggestion. Don't allocate memory for this, it's a waste of effort.

Since app_cfg is a file-level variable, you can only have one copy at a time anyway, so there's little point in allocating and de-allocating it.

Just create it as a static non-pointer and use it:

static struct Config_t {
    char protocol[LINE_SIZE];
    char mode[LINE_SIZE];
} app_cfg;

You can still provide a create and destroy which memset the structure to zeros but even that may not be required:

void create_object (void) {
    memset(&app_cfg, 0, sizeof(app_cfg));
}

void destroy_config (void) {
    memset(&app_cfg, 0, sizeof(app_cfg));
}
paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
  • `memset` definitely isn't needed. It is guaranteed to be 0-filled, since it's static. And destroy doesn't seem to make sense either. – Matthew Flaschen Nov 22 '10 at 07:07
  • @Matthew, I was thinking more of the case where you wanted it cleared before _reuse_ rather than initial use. But, even if it's _not_ cleared on create/destroy, decent coders shouldn't be writing code that's affected by values left lying around, hence my "may not be required" comment. – paxdiablo Nov 22 '10 at 07:09
  • I have changed my source code and done as you suggested. Just one question. In my source code I will only need one copy of the struct variable (stack level, static global). I guess if I was creating many objects of this structure and wanted them to be available for the life-time of the application. I guess that is a good reason for using malloc? – ant2009 Nov 22 '10 at 07:13
  • you have a point about possible reuse (though I don't know that a method is needed). – Matthew Flaschen Nov 22 '10 at 07:14
  • 2
    @ant2009: yes. If you wanted multiple copies, you would them malloc them and return the pointer from `create_whatever`. It would then be useful to have a `destroy_whatever` to clean up since that means you would be able to change that code at will without affecting the API. – paxdiablo Nov 22 '10 at 07:21
2

using this code with gcc 3.3.3 under Cygwin works correctly for me when I call it twice. You didn't tell us what you're doing outside of these functions, so look there first, e.g. maybe you're accidentally assigning a garbage non-NULL value to app_cfg between calls. Also, if you're not using a "big-name" compiler, there's a possibility this is a compiler bug (e.g. it may be overly optimistic at compile time and assume you'll never pass a NULL to destroy_config). Try putting in something like:

void destroy_config()
{

    /* Check to see if the memory is ok to free */
    if(app_cfg != NULL) {
        printf("not null\n" );
        free(app_cfg);
        app_cfg = NULL;
    }else{
        printf("null\n" );
        }
}

to see if it really "knows" when it's null.

Sam Skuce
  • 1,666
  • 14
  • 20