Hi Antonio,
first, I'm just using the nortel branch because it is used in the
openSUSE vpnc package. It provides additional features compared with
trunk and Novell used a Nortel gateway back when I was at SUSE, so that
was a good reason to package a compatible vpnc ;-)
I suspect that all the found issues are also present in trunk, and I can
verify this and provide patches for trunk, too.
This is valid for all the proposed patches.
On 08.11.2011 03:44, Antonio Borneo wrote:
> On Mon, Nov 7, 2011 at 10:07 PM, Stefan Seyfried
> <stefan.seyfried@googlemail.com> wrote:
> The memory leaks you have found reveal bad coding inside make_our_sa_ipsec().
>
>> case ISAKMP_PAYLOAD_N:
>> free(p->u.n.spi);
>> free(p->u.n.data);
>> + free_isakmp_attributes(p->u.n.attributes);
>> break;
> Agree!
This one was a bit harder to find, as this was a recursive allocation
and the valgrind output was not that easy to interpret. Additionally,
something was freed, just not everything ;)
>> static struct isakmp_payload *make_our_sa_ipsec(struct sa_block *s)
>> {
>> - struct isakmp_payload *r = new_isakmp_payload(ISAKMP_PAYLOAD_SA);
>> + struct isakmp_payload *r;
>> struct isakmp_payload *p = NULL, *pn;
> Agree!
This one was easy and trivial ;-)
>> for (i = 0, pn = p; pn; pn = pn->next)
>> pn->u.p.number = i++;
>> + free_isakmp_payload(r->u.sa.proposals);
>> r->u.sa.proposals = p;
>> return r;
> Here the issue is more complex.
> The whole content of "r->u.sa.proposals" is allocated and populated at
> the beginning of the procedure, but then never used and replaced by
> "p".
> Agree that allocated memory should be freed before loosing the
> pointer, but in this case there is useless code that should be
> removed.
> By removing the whole allocation of "r->u.sa.proposals" this call to
> free_isakmp_payload() is not required any longer.
Ok. I was simply doing an "error driven" approach: looking at the
memleak errors in valgrind, I simply fixed the issues in the most easy
way, which was to free the allocated resource at that point, as it
obviously was not used anymore ;-)
> Would you like to provide a patch?
I can try.
> The same bad code is in trunk too!
As lined out above, I'm using the nortel branch just because it is more
convenient to test for me. I can later port the patches to trunk, too.
>> extern struct isakmp_payload *dup_isakmp_payload(struct isakmp_payload *p);
>> -//extern void free_isakmp_payload(struct isakmp_payload *p);
>> +extern void free_isakmp_payload(struct isakmp_payload *p);
>> extern struct isakmp_attribute *new_isakmp_attribute(uint16_t, struct isakmp_attribute *);
> Since there is no need to call free_isakmp_payload(), this part can be removed.
Of course.
--
Stefan Seyfried
Linux Consultant & Developer
Mail: seyfried@b1-systems.de GPG Key: 0x731B665B
B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg /
http://www.b1-systems.de GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt,HRB 3537
_______________________________________________
vpnc-devel mailing list
vpnc-devel@unix-ag.uni-kl.de
https://lists.unix-ag.uni-kl.de/mailman/listinfo/vpnc-devel http://www.unix-ag.uni-kl.de/~massar/vpnc/
vpnc-fix-valgrind-warnings.diff (2.60 KB)