Bug 740413 - do_free() cleanup
Summary: do_free() cleanup
Keywords:
Status: NEW
Alias: None
Product: Dogtag Certificate System
Classification: Retired
Component: TPS
Version: 9.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Christina Fu
QA Contact: Ben Levenson
URL:
Whiteboard:
Depends On:
Blocks: 530474
TreeView+ depends on / blocked
 
Reported: 2011-09-21 23:24 UTC by Andrew Wnuk
Modified: 2015-01-05 00:27 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:


Attachments (Terms of Use)

Description Andrew Wnuk 2011-09-21 23:24:22 UTC
Current do_free() implementation is incorrect:

    inline void do_free(char * buf)
    {
        if (buf != NULL) {
            PR_Free(buf);
            buf = NULL;
        }
    }

buf variable is passed by value not by reference. This means that the value of the variable in the calling function does not change.
The right approach would be:

    inline void do_free(char ** buf)
    {
        if ((buf != NULL) && (*buf != NULL)) {
            PR_Free(*buf);
            *buf = NULL;
        }
    }

       ...

    do_free(&buf);



Or do a macro and do NULL assignment after the do_free() returns.

#define DO_FREE(x)  { do_free(x); x = NULL; }

Right now double free will cause a crash as the variable in the calling function is not cleared.
Also as far as I know it is a good practice nowadays to leave it to the optimizer to decide which functions to inline and which not.




There are 242 occurrences on which do_free() is used in three files:
  pki/base/migrate/TpsTo80/migrateTPSData.c
  pki/base/tps/src/engine/RA.cpp
  pki/base/tps/src/modules/tokendb/mod_tokendb.cpp

find pki -name '*.c*' | xargs grep do_free | grep -v svn
242

Comment 1 Dmitri Pal 2011-09-21 23:45:33 UTC
another alternative would be:

char *do_free(char * buf)
{
    if (buf != NULL) {
        PR_Free(buf);
        buf = NULL;
    }
    return buf;
}

...

buf = do_free(buf);

and then macro can be defined like this:

#define DO_FREE(foo) { foo = do_free(foo); }

Please check with SSSD team on the best approach.

Comment 2 Stephen Gallagher 2011-09-22 12:04:20 UTC
Assuming that PR_Free(buf) can never fail (aka will never return without freeing memory) then the best approach is probably to just use a macro like

#define do_zfree(ptr) do { \
        do_free(ptr); \
        ptr = NULL; \
    } while(0)

(This construct allows you to make the macro look like a standard function, since it must be followed by a semicolon).

So in your code you would just call
do_zfree(ptr);


Finally, your if (buf != NULL) check is probably in the wrong place. PR_Free() should be designed so that it's a no-op if it is passed NULL. If it's not, that's probably a bug.

Comment 3 Dmitri Pal 2011-09-23 02:35:20 UTC
> Finally, your if (buf != NULL) check is probably in the wrong place. PR_Free()
> should be designed so that it's a no-op if it is passed NULL. If it's not,
> that's probably a bug.

I think PR_xxx is an external interface so we can't do any assumptions about it thus need to always check before calling it. IMO with this twist the check is required and in the right place.

On the other part... Ah, I forgot about the while(0) and the issue with ";" not being a noop on some compilers. Is it still the case and should still be considered? What is the chance to ever encounter such constraint especially in the Dogtag project?

Comment 4 Jakub Hrozek 2011-09-23 11:11:24 UTC
(In reply to comment #3)
> On the other part... Ah, I forgot about the while(0) and the issue with ";" not
> being a noop on some compilers. Is it still the case and should still be
> considered? What is the chance to ever encounter such constraint especially in
> the Dogtag project?

The purpose of wrapping a multi-line macro in a "do { } while(0)" (no semicolon) is to make the macro usable as a regular statement, not a compound statement.

When the while(0) is terminated with a semicolon, the "do { } while(0);" block becomes a compound statement again.

We've had a couple of bad macro definitions in SSSD as well, a patch is on the list.

Comment 5 Dmitri Pal 2011-09-26 23:52:42 UTC
I thought that the only reason is that if you by mistake put extra ";" after the macro that ends with ";" some of the old compilers would choke. My point that with the modern compilers that should not be the case and second ";" will be treated as no op. 
Is there any reason other than make it look like a function and allow to put the ";" at the end with any compiler why "do { } while(0)" is used?

Comment 6 Jakub Hrozek 2011-09-27 08:54:30 UTC
We might actually be talking about the same thing, but I think it applies to modern compilers as well.

Consider the following program:

-----
#include <stdio.h>

#define PRINT(i) do {          \
    printf("i is %d\n", i);    \
} while(0)                     \

int main(void)
{
    int i = 5;

    if (i > 0)
        PRINT(i);
    else
        PRINT(i*-1);

    return 0;
}
-------

This compiles fine, but if you used "while(0);", the code no longer compiles, because the macro would expand to:

-------
if (i > 0)
  do { printf("i is %d\n", i); } while(0); ;
-------

which is a compound ("multiline") statement that must be enclosed in { }


Note You need to log in before you can comment on or make changes to this bug.