From: Wolfgang S. <wol...@di...> - 2011-11-07 18:44:59
Attachments:
p2-p1_memory_leak_fixes_parser.patch.tar.bz2
|
Dear ipsec-tools maintainers, I am providing a patch which fixes several memory leaks upon configuration file parsing. The main purpose is to avoid, that memory leaks accumulate upon configuration reload. I did introduce several macros starting with ABORT_PARSE.., which are now used instead of exiting with return -1. Those macros take care, that memory in cur_rmconf and cur_sainfo is released, before the parser exits. There are several additional memory leak fixes independent of those macros. My patch also supports the consequent use of calling yywarn instead of yyerror, in case the parsing is not aborted. This happens typically when a feature is being configured, but not compiled in. The patch is based on a CVS trunk snapshot from yesterday evening: an...@an...:/cvsroot at 2011-11-06 22:00h MEZ. It can be applied independent of my previous patch p1-2011-11-06_rename_pfkey_to_racoon_pfkey.patch.tar.bz2 since the changed files are not in conflict. Regards Wolfgang Schmieder |
From: Timo T. <tim...@ik...> - 2011-11-12 12:11:05
|
On 11/07/2011 08:43 PM, Wolfgang Schmieder wrote: > I am providing a patch which fixes several memory leaks upon configuration > file parsing. The main purpose is to avoid, that memory leaks accumulate > upon configuration reload. Very nice, seems you have various valid things here. General first notes: - use ANSI C comments (/* */), C++ comments are not acceptable (//) - do not add curly braces / blocks unless needed, especially if that's the only change within context - most of the code uses no braces in after "if ()" if not necessary. - keep a space between "if" and "()"; some old code does not have it, but all new code should > I did introduce several macros starting with ABORT_PARSE.., which are now > used instead of exiting with return -1. Those macros take care, that memory > in cur_rmconf and cur_sainfo is released, before the parser exits. There are > several additional memory leak fixes independent of those macros. +#define ABORT_CLEANUP {delrmconf(cur_rmconf); delsainfo(cur_sainfo);YYABORT;} +#define ABORT_PARSE(yyerr) {yyerr; ABORT_CLEANUP} The 'yyerr' of these macros is just a regular statement. It makes no sense to have it as macro parameter like that. Either: a) remove 'yyerr' there, and when needed have it as separate statement before the macro invocation b) since most use yyerr as call to yyerror, call the yyerror unconditionally and just pass the string here. It would improve the code too if an error was always printed. +#define ABORT_PARSE_VFREE_1(yyerr, val0) {yyerr; \ + vfree(val0); val0 = NULL;\ + ABORT_CLEANUP} + +#define ABORT_PARSE_FREE_1(yyerr, val0) {yyerr; \ + racoon_free(val0); val0 = NULL;\ + ABORT_CLEANUP} Perhaps call these: ABORT_AND_VFREE ABORT_AND_VFREE2 ABORT_AND_RACOON_FREE ABORT_AND_RACOON_FREE2 Or something similar? I was first also thinking if these could be functions instead, but it might be tricky. > My patch also supports the consequent use of calling yywarn instead of > yyerror, in case the parsing is not aborted. This happens typically when a > feature is being configured, but not compiled in. Regarding: +static const char szEnableHybridCfgWarn[] = "racoon not configured with --enable-hybrid\n"; and others, we don't use the hungarian notation. Please rewrite the variables to something else like error_message_hybrid_config_not_configured. You also have changes like: - yywarn("admin port support not compiled in"); + yywarn("szEnableAdminPortCfgWarn"); The new yywarn should not probably have quotation marks as you very intending to use the previously defined global strings, right? The part (in two different places of the patch) with: + struct idspec *idspec = NULL; + { + vchar_t* id = NULL; + if (set_identifier(&id, $2, $3) != 0) { Seems to add the extra curly brace block after 'idspec' definition for no apparent reason. That should not be needed. > The patch is based on a CVS trunk snapshot from yesterday evening: > an...@an...:/cvsroot at 2011-11-06 22:00h MEZ. It can be > applied independent of my previous patch > p1-2011-11-06_rename_pfkey_to_racoon_pfkey.patch.tar.bz2 since the changed > files are not in conflict. You also sent the later patch that fixes things this patch introduces. Could those changes be merged to this one, so we don't have to commit things that are known to be wrong. (E.g. I use often git import to review logs, do bisecting etc.). Otherwise the bulk of the patch looks ok. Thanks, Timo |
From: Wolfgang S. <wol...@di...> - 2011-11-17 18:51:42
|
Timo, I am attaching Version2 of my patch p2 with the changes you proposed in your email below. The patch is based on the an...@an...:/cvsroot at 2011-11-17 17:00h MEZ. Regards Wolfgang -----Ursprüngliche Nachricht----- Von: Timo Teräs [mailto:tim...@gm...] Im Auftrag von Timo Teräs Gesendet: Samstag, 12. November 2011 13:11 An: Wolfgang Schmieder Cc: ips...@li... Betreff: Re: [Ipsec-tools-devel] Memory leak fixes On 11/07/2011 08:43 PM, Wolfgang Schmieder wrote: > I am providing a patch which fixes several memory leaks upon configuration > file parsing. The main purpose is to avoid, that memory leaks accumulate > upon configuration reload. Very nice, seems you have various valid things here. General first notes: - use ANSI C comments (/* */), C++ comments are not acceptable (//) - do not add curly braces / blocks unless needed, especially if that's the only change within context - most of the code uses no braces in after "if ()" if not necessary. - keep a space between "if" and "()"; some old code does not have it, but all new code should > I did introduce several macros starting with ABORT_PARSE.., which are now > used instead of exiting with return -1. Those macros take care, that memory > in cur_rmconf and cur_sainfo is released, before the parser exits. There are > several additional memory leak fixes independent of those macros. +#define ABORT_CLEANUP {delrmconf(cur_rmconf); delsainfo(cur_sainfo);YYABORT;} +#define ABORT_PARSE(yyerr) {yyerr; ABORT_CLEANUP} The 'yyerr' of these macros is just a regular statement. It makes no sense to have it as macro parameter like that. Either: a) remove 'yyerr' there, and when needed have it as separate statement before the macro invocation b) since most use yyerr as call to yyerror, call the yyerror unconditionally and just pass the string here. It would improve the code too if an error was always printed. +#define ABORT_PARSE_VFREE_1(yyerr, val0) {yyerr; \ + vfree(val0); val0 = NULL;\ + ABORT_CLEANUP} + +#define ABORT_PARSE_FREE_1(yyerr, val0) {yyerr; \ + racoon_free(val0); val0 = NULL;\ + ABORT_CLEANUP} Perhaps call these: ABORT_AND_VFREE ABORT_AND_VFREE2 ABORT_AND_RACOON_FREE ABORT_AND_RACOON_FREE2 Or something similar? I was first also thinking if these could be functions instead, but it might be tricky. > My patch also supports the consequent use of calling yywarn instead of > yyerror, in case the parsing is not aborted. This happens typically when a > feature is being configured, but not compiled in. Regarding: +static const char szEnableHybridCfgWarn[] = "racoon not configured with --enable-hybrid\n"; and others, we don't use the hungarian notation. Please rewrite the variables to something else like error_message_hybrid_config_not_configured. You also have changes like: - yywarn("admin port support not compiled in"); + yywarn("szEnableAdminPortCfgWarn"); The new yywarn should not probably have quotation marks as you very intending to use the previously defined global strings, right? The part (in two different places of the patch) with: + struct idspec *idspec = NULL; + { + vchar_t* id = NULL; + if (set_identifier(&id, $2, $3) != 0) { Seems to add the extra curly brace block after 'idspec' definition for no apparent reason. That should not be needed. > The patch is based on a CVS trunk snapshot from yesterday evening: > an...@an...:/cvsroot at 2011-11-06 22:00h MEZ. It can be > applied independent of my previous patch > p1-2011-11-06_rename_pfkey_to_racoon_pfkey.patch.tar.bz2 since the changed > files are not in conflict. You also sent the later patch that fixes things this patch introduces. Could those changes be merged to this one, so we don't have to commit things that are known to be wrong. (E.g. I use often git import to review logs, do bisecting etc.). Otherwise the bulk of the patch looks ok. Thanks, Timo |
From: Timo T. <tim...@ik...> - 2012-01-01 15:30:17
|
Hi Wolfgang, Sorry for the latency. Applied to CVS. Thanks, Timo On 11/17/2011 08:49 PM, Wolfgang Schmieder wrote: > I am attaching Version2 of my patch p2 with the changes you proposed in your > email below. > > The patch is based on the an...@an...:/cvsroot at 2011-11-17 > 17:00h MEZ. > > Regards > Wolfgang > > -----Ursprüngliche Nachricht----- > Von: Timo Teräs [mailto:tim...@gm...] Im Auftrag von Timo Teräs > Gesendet: Samstag, 12. November 2011 13:11 > An: Wolfgang Schmieder > Cc: ips...@li... > Betreff: Re: [Ipsec-tools-devel] Memory leak fixes > > On 11/07/2011 08:43 PM, Wolfgang Schmieder wrote: >> I am providing a patch which fixes several memory leaks upon configuration >> file parsing. The main purpose is to avoid, that memory leaks accumulate >> upon configuration reload. > > Very nice, seems you have various valid things here. > > General first notes: > - use ANSI C comments (/* */), C++ comments are not acceptable (//) > - do not add curly braces / blocks unless needed, especially if that's > the only change within context > - most of the code uses no braces in after "if ()" if not necessary. > - keep a space between "if" and "()"; some old code does not have it, > but all new code should > >> I did introduce several macros starting with ABORT_PARSE.., which are now >> used instead of exiting with return -1. Those macros take care, that > memory >> in cur_rmconf and cur_sainfo is released, before the parser exits. There > are >> several additional memory leak fixes independent of those macros. > > +#define ABORT_CLEANUP {delrmconf(cur_rmconf); > delsainfo(cur_sainfo);YYABORT;} > +#define ABORT_PARSE(yyerr) {yyerr; ABORT_CLEANUP} > > The 'yyerr' of these macros is just a regular statement. It makes no > sense to have it as macro parameter like that. > > Either: > a) remove 'yyerr' there, and when needed have it as separate statement > before the macro invocation > b) since most use yyerr as call to yyerror, call the yyerror > unconditionally and just pass the string here. It would improve > the code too if an error was always printed. > > +#define ABORT_PARSE_VFREE_1(yyerr, val0) {yyerr; \ > + vfree(val0); val0 = NULL;\ > + ABORT_CLEANUP} > + > +#define ABORT_PARSE_FREE_1(yyerr, val0) {yyerr; \ > + racoon_free(val0); val0 = NULL;\ > + ABORT_CLEANUP} > > Perhaps call these: > ABORT_AND_VFREE > ABORT_AND_VFREE2 > ABORT_AND_RACOON_FREE > ABORT_AND_RACOON_FREE2 > > Or something similar? > > I was first also thinking if these could be functions instead, but it > might be tricky. > >> My patch also supports the consequent use of calling yywarn instead of >> yyerror, in case the parsing is not aborted. This happens typically when a >> feature is being configured, but not compiled in. > > Regarding: > > +static const char szEnableHybridCfgWarn[] = "racoon not configured with > --enable-hybrid\n"; > > and others, we don't use the hungarian notation. Please rewrite the > variables to something else like error_message_hybrid_config_not_configured. > > You also have changes like: > > - yywarn("admin port support not compiled in"); > + yywarn("szEnableAdminPortCfgWarn"); > > The new yywarn should not probably have quotation marks as you very > intending to use the previously defined global strings, right? > > The part (in two different places of the patch) with: > + struct idspec *idspec = NULL; > + { > + vchar_t* id = NULL; > + if (set_identifier(&id, $2, $3) != 0) { > > Seems to add the extra curly brace block after 'idspec' definition for > no apparent reason. That should not be needed. > >> The patch is based on a CVS trunk snapshot from yesterday evening: >> an...@an...:/cvsroot at 2011-11-06 22:00h MEZ. It can be >> applied independent of my previous patch >> p1-2011-11-06_rename_pfkey_to_racoon_pfkey.patch.tar.bz2 since the changed >> files are not in conflict. > > You also sent the later patch that fixes things this patch introduces. > Could those changes be merged to this one, so we don't have to commit > things that are known to be wrong. (E.g. I use often git import to > review logs, do bisecting etc.). > > Otherwise the bulk of the patch looks ok. > > Thanks, > Timo |