C++

Forum

 
 
How to properly free() a _strdup variable

xwielder (53)
How to properly free() a _strdup variable?

Here's what I have:

File prof.cpp 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
 
// Local function
void free_proID(struct prof_Name_ID *proID);

// Local const int
const int limit = 50;

// Local struct
struct prof_Name_ID
{
    char *_class[limit];
    char *name[limit];
    char *bonus[limit];
};

// Local #define
#define CREATE(result, type, number)  \
    do \
    { \
        if ((number) * sizeof(type) <= 0)   \
            LOG(SYSTEMLOG, "[CREATE-ERROR]", "SYSERR: Zero bytes or less requested at %s:%d.", __FILE__, __LINE__); \
        if (!((result) = (type *) calloc ((number), sizeof(type)))) \
        { \
            LOG(SYSTEMLOG, "[SYSERR: malloc failure]", "SYSERR: malloc failure"); \
            perror("SYSERR: malloc failure"); \
            abort(); \
        } \
    } while(0)

void testFunction()
{
    MySQL my;
    char szQueryText[500];
    MYSQL_RES *My_result = NULL;
    
    struct prof_Name_ID *proID = NULL;
    CREATE(proID, struct prof_Name_ID, 1);
    
    // Initialize and null out local struct prof_Name_ID
    for (int a = 0; a < limit; a++)
    {
        proID->_class[a] = "";
        proID->name[a] = "";
        proID->bonus[a] = "";
    }
    
    // Pull data in from MySQL
    snprintf(szQueryText, sizeof(szQueryText), "SELECT * FROM someTable WHERE active = 1 ORDER BY rowID ASC;");
    My_result = MySQL__query(szQueryText);
    int num = my.Query(szQueryText);
    
    if (num > limit)
        return;
    
    while (num > 0)
    {
        my.NextRow();
        num--;

        proID->_class[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionClass"));
        proID->name[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionName"));
        proID->bonus[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionBonus"));
    }
    MySQL__endquery(My_result);
    
    // Okay, now free() the data we _strdup'd
    free_proID(proID);
}

void free_proID(struct prof_Name_ID *proID)
{
    for (int a = 0; a < limit; a++)
    {
        proID->_class[a] = NULL;
        proID->name[a] = NULL;
        proID->bonus[a] = NULL;
        
        if (proID->_class[a])
            free(proID->_class[a]);
        if (proID->name[a])
            free(proID->name[a]);
        if (proID->bonus[a])
            free(proID->bonus[a]);
    }

    if (proID)
    {
        proID = NULL;
        free(proID);
    }
}


All code works fine, but I'm not feeling comfortable with the free_proID() function. Am I free()ing what I _strdup'd correctly? Could it be done better?

Thank you all for looking.
ne555 (10692)
You are doing nothing but leaking memory.
The conditions in line 77,79,81 would always evaluate to false.
Last edited on
xwielder (53)
Okay, so:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
 
void free_proID(struct prof_Name_ID *proID)
{
    for (int a = 0; a < limit; a++)
    {
        if (proID->_class[a])
            free(proID->_class[a]);
        if (proID->name[a])
            free(proID->name[a]);
        if (proID->bonus[a])
            free(proID->bonus[a]);
    }

    if (proID)
    {
        proID = NULL;
        free(proID);
    }
}


Yes? No? (since you didn't provide an option)
xwielder (53)
With the change I've posted above, I now get the following compile warnings:

warning C6001: Using uninitialized memory '*proID[BYTE:4]'.:
warning C6001: Using uninitialized memory '*proID[BYTE:204]'.:
warning C6001: Using uninitialized memory '*proID[BYTE:404]'.:
ne555 (10692)
1
2
 
        proID = NULL; //leak
        free(proID); //does nothing
Also, changes to `proID' would be local to the function.
And if line 13 were false, the loop is an error

By the way, as you are using c++, ¿why don't you use std::string?
Last edited on
xwielder (53)
So, if I'm understanding you correctly, it should simply be:

1
2
3
4
5
 
void free_proID(struct prof_Name_ID *proID)
{
    if (proID)
        free(proID);
}


Correct?
xwielder (53)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
 
// Local function
void free_proID(struct prof_Name_ID *proID);

// Local const int
const int limit = 50;

// Local struct
struct prof_Name_ID
{
    char *_class[limit];
    char *name[limit];
    char *bonus[limit];
};

// Local #define
#define CREATE(result, type, number)  \
    do \
    { \
        if ((number) * sizeof(type) <= 0)   \
            LOG(SYSTEMLOG, "[CREATE-ERROR]", "SYSERR: Zero bytes or less requested at %s:%d.", __FILE__, __LINE__); \
        if (!((result) = (type *) calloc ((number), sizeof(type)))) \
        { \
            LOG(SYSTEMLOG, "[SYSERR: malloc failure]", "SYSERR: malloc failure"); \
            perror("SYSERR: malloc failure"); \
            abort(); \
        } \
    } while(0)

void testFunction()
{
    MySQL my;
    char szQueryText[500];
    MYSQL_RES *My_result = NULL;
    
    struct prof_Name_ID *proID = NULL;
    CREATE(proID, struct prof_Name_ID, 1);
    
    // Initialize and null out local struct prof_Name_ID
    for (int a = 0; a < limit; a++)
    {
        proID->_class[a] = _strdup("");
        proID->name[a] = _strdup("");
        proID->bonus[a] = _strdup("");
    }
    
    // Pull data in from MySQL
    snprintf(szQueryText, sizeof(szQueryText), "SELECT * FROM someTable WHERE active = 1 ORDER BY rowID ASC;");
    My_result = MySQL__query(szQueryText);
    int num = my.Query(szQueryText);
    
    if (num > limit)
        return;
    
    while (num > 0)
    {
        my.NextRow();
        num--;

        if (proID->_class[atoi(my.GetData("professionID"))])
            free(proID->_class[atoi(my.GetData("professionID"))]);
        proID->_class[atoi(my.GetData("professionID"))] = NULL;
        
        if (proID->name[atoi(my.GetData("professionID"))])
            free(proID->name[atoi(my.GetData("professionID"))]);
        proID->name[atoi(my.GetData("professionID"))] = NULL;
        
        if (proID->bonus[atoi(my.GetData("professionID"))])
            free(proID->bonus[atoi(my.GetData("professionID"))]);
        proID->bonus[atoi(my.GetData("professionID"))] = NULL;

        proID->_class[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionClass"));
        proID->name[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionName"));
        proID->bonus[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionBonus"));
    }
    
    // Okay, now free() the data we _strdup'd
    free_proID(proID);
}

void free_proID(struct prof_Name_ID *proID)
{
    for (int a = 0; a < limit; a++)
    {
        if (proID->_class[a])
            free(proID->_class[a]);
        if (proID->name[a])
            free(proID->name[a]);
        if (proID->bonus[a])
            free(proID->bonus[a]);

        proID->_class[a] = NULL;
        proID->name[a] = NULL;
        proID->bonus[a] = NULL;
    }

    if (proID)
    {        
        free(proID);
        proID = NULL;
    }
}


Above code is what it should be.

First, instead of initializing the struct like this:
1
2
3
4
5
6
7
 
// Initialize and null out local struct prof_Name_ID
    for (int a = 0; a < limit; a++)
    {
        proID->_class[a] = "";
        proID->name[a] = "";
        proID->bonus[a] = "";
    }


I am using _strdup:
1
2
3
4
5
6
7
 
// Initialize and null out local struct prof_Name_ID
    for (int a = 0; a < limit; a++)
    {
        proID->_class[a] = _strdup("");
        proID->name[a] = _strdup("");
        proID->bonus[a] = _strdup("");
    }


Second, the free_proID() function works flawlessly and doesn't leak anymore.
Last edited on
ne555 (10692)
Consider free_proID( NULL );

Line 99 is useless.

¿why don't you simply initialize to NULL?
Also, you should encapsulate 59-69
andywestken (4087)
The null test for proID is too late as you've already (tried to) dereference the pointer on line 84

Try...

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
 
void free_proID(struct prof_Name_ID *proID)
{
    if (!proID) // make sure the pointer's not null before we use it
        return; // (it's ok to free a null pointer, but not "->" it)

    for (int a = 0; a < limit; a++)
    {
        if (proID->_class[a]) // proID-> will blow up if proID is null
            free(proID->_class[a]);
        if (proID->name[a])
            free(proID->name[a]);
        if (proID->bonus[a])
            free(proID->bonus[a]);

        // no point nulling these as you're just about
        // to delete the parent structure
        //proID->_class[a] = NULL;
        //proID->name[a] = NULL;
        //proID->bonus[a] = NULL;

        // Aside: the only time you need to zero memory you're going
        // to free in really security conscious code, where you scrub the
        // the memory to ensure no one can see any useful in memory
        // using a debugger.
    }
    
    free(proID);
    // no point as proID passed by value
    //proID = NULL;
}


As it is safe to pass null to free() (and to delete), this can be simplified to

1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
void free_proID(struct prof_Name_ID *proID)
{
    if (!proID)
        return;

    for (int a = 0; a < limit; a++)
    {
        free(proID->_class[a]);
        free(proID->name[a]);
        free(proID->bonus[a]);
    }
    
    free(proID);
}


Or, if you want free_proID to null the caller's pointer, in C++ code you can use a reference to a pointer:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
 
void free_and_null_proID(struct prof_Name_ID *&proID)
{
    if (!proID)
        return;

    for (int a = 0; a < limit; a++)
    {
        free(proID->_class[a]);
        free(proID->name[a]);
        free(proID->bonus[a]);
    }
    
    free(proID);
    proID = NULL; // now works as proID passed by reference to pointer
}


or C version, using a pointer to a pointer

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
 
// this version called like this: free_and_null_proID(&proID);
void free_and_null_proID(struct prof_Name_ID **pproID)
{
    if (!pproID) // check pointer to pointer isn't null
        return;

    if (!(*pproID)) // and pointer isn't null
        return;

    for (int a = 0; a < limit; a++)
    {
        free((*pproID)->_class[a]);
        free((*pproID)->name[a]);
        free((*pproID)->bonus[a]);
    }
    
    free((*pproID));
    (*pproID)  = NULL; // now works as proID passed by pointer to pointer
}


Andy

PS I agree with ne555 that nulling the elements of the arrays in your struct prof_Name_ID would make more sense. It could even be done using memset:

memset(proID, 0, sizeof(struct prof_Name_ID)); // C

memset(proID, 0, sizeof(prof_Name_ID)); // C++

And that your strdup-loop could drop the ifs

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
 
    while (num > 0)
    {
        my.NextRow();
        num--;

        free(proID->_class[atoi(my.GetData("professionID"))]);
        proID->_class[atoi(my.GetData("professionID"))] = NULL;
        
        free(proID->name[atoi(my.GetData("professionID"))]);
        proID->name[atoi(my.GetData("professionID"))] = NULL;
        
        free(proID->bonus[atoi(my.GetData("professionID"))]);
        proID->bonus[atoi(my.GetData("professionID"))] = NULL;

        proID->_class[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionClass"));
        proID->name[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionName"));
        proID->bonus[atoi(my.GetData("professionID"))] = _strdup(my.GetData("professionBonus"));
    }


Though this code looks in severe need of a refactoring: noticing the frequent use of the numeric profession ID, which I assume is constant (at least for a given pass of the loop.)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
    while (num > 0)
    {
        my.NextRow();
        num--;

        // if C code, declare in appropriate place
        int professionID = atoi(my.GetData("professionID"));

        free(proID->_class[professionID]);
        proID->_class[professionID] = NULL;
        
        free(proID->name[professionID]);
        proID->name[professionID] = NULL;
        
        free(proID->bonus[professionID]);
        proID->bonus[professionID] = NULL;

        proID->_class[professionID] = _strdup(my.GetData("professionClass"));
        proID->name[professionID] = _strdup(my.GetData("professionName"));
        proID->bonus[professionID] = _strdup(my.GetData("professionBonus"));
    }
Last edited on
xwielder (53)
Andy, awesome insight, and thank you!

Allow me to explain my position on NULLing.

If I free() and don't NULL it out, another iteration would attempt to free() it again, causing a crash.

For example:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
void free_proID(struct prof_Name_ID *proID)
{
    // First, this is a great check you've suggested and can't believe
    // I missed not having it in there before.
    if (!proID)
        return;

    for (int a = 0; a < limit; a++)
    {
        if (proID->_class[a]) // If this was free()'d before and not NULLed out, it will still equate to true
            free(proID->_class[a]);  // thus, this free() would cause program to crash.
        
        // I must NULL this out after it's been free()'d so that any other
        // *possible* attempt at free()'ing it would fail.  If it didn't fail,
        // the program would crash.
        proID->_class[a] = NULL;
    }
    
    if (proID)
    {
        free(proID);
        proID = NULL;
    }


In regards to your refactoring comment, true, it could easily done int professionID = atoi(my.GetData("professionID"));, but is there any harm in not?

ne555 (10692)
Again, line 22 does nothing. The change is local to the function, you are not affecting the parameter
1
2
3
4
5
6
7
8
9
 
#include <iostream>
void foo(int n){
   n = 0; //changing a local variable
}

int main(){
   int n = 42;
   std::cout << n << '\n';
}
 
42


> memset(proID, 0, sizeof(prof_Name_ID)); // C++
Use a constructor instead
cire (8284)
If I free() and don't NULL it out, another iteration would attempt to free() it again, causing a crash.


In your version of the code, proID is a local variable that ceases existing when the function returns, so you set it to NULL and then it stops existing. In other words, setting proID to NULL on line 22 has no effect at all (and, in fact, it will be removed by any decent optimizer.)
xwielder (53)
[quote="ne555"]Use a constructor instead [/quote]

I agree too. 
1
2
3
4
5
 
if (proID)
{        
    free(proID);
    memset(proID, 0, sizeof(prof_Name_ID));
}


Nice catch.
cire (8284)
1
2
3
4
5
 
if (proID)
{        
    free(proID);
    memset(proID, 0, sizeof(prof_Name_ID));
}


This still doesn't affect the caller's pointer value, so it's rather pointless (pardon the pun.) Maybe you should re-read Andy's last post.
xwielder (53)
True. Thank you.
void free_proID(struct prof_Name_ID*&proID) works great.
Last edited on
Topic archived. No new replies allowed.