Re: [Ipsec-tools-devel] Memory leak fixes

[Timo T. <tim...@ik...>]

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