[prev in list] [next in list] [prev in thread] [next in thread] 

List:       ipsec-tools-devel
Subject:    Re: [Ipsec-tools-devel] Memory leak fixes
From:       "Wolfgang Schmieder" <wolfgang () die-schmieders ! de>
Date:       2011-11-17 18:49:52
Message-ID: BEB9187FD0EF4BE88F046C9959A290C0 () Avitos
[Download RAW message or body]

Timo,

I am attaching Version2 of my patch p2 with the changes you proposed in your
email below.

The patch is based on the anoncvs@anoncvs.netbsd.org:/cvsroot at 2011-11-17
17:00h MEZ.

Regards
	Wolfgang

-----Ursprüngliche Nachricht-----
Von: Timo Teräs [mailto:timo.teras@gmail.com] Im Auftrag von Timo Teräs
Gesendet: Samstag, 12. November 2011 13:11
An: Wolfgang Schmieder
Cc: ipsec-tools-devel@lists.sourceforge.net
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:
> anoncvs@anoncvs.netbsd.org:/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

["V2_p2-2011-11-17_memory_leak_fixes_parser.patch.tar.bz2" (application/octet-stream)]

------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure 
contains a definitive record of customers, application performance, 
security threats, fraudulent activity, and more. Splunk takes this 
data and makes sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-novd2d

_______________________________________________
Ipsec-tools-devel mailing list
Ipsec-tools-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipsec-tools-devel


[prev in list] [next in list] [prev in thread] [next in thread]