Hide Forgot
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
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.
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.
> 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?
(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.
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?
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 { }