Skip Menu |
 

Date: Sat, 11 Mar 2017 16:09:18 -0500
From: Cel Skeggs <cela@mit.edu>
To: krb5-bugs@mit.edu
Subject: kvno memory leak (1.15.1)
Download (untitled) / with headers
text/plain 2.2KiB
Hi!

I think I've found a memory leak in kvno, as of 1.15.1. I had trouble
getting access to the development snapshots, so please disregard this
email if it's already been fixed since 1.15.1.

To reproduce:

$ valgrind kvno zephyr/zephyr HTTP/roost-api.mit.edu

You should also be able to use any other valid invocation of kvno, but
the leak will be most obvious when more than one service is included in
a single invocation.

Part of the result:

LEAK SUMMARY:
definitely lost: 245 bytes in 4 blocks
indirectly lost: 891 bytes in 23 blocks
possibly lost: 0 bytes in 0 blocks
still reachable: 1,181 bytes in 27 blocks
suppressed: 0 bytes in 0 blocks

Likely cause:

In kvno.c, the snippet of code that frees the per-service
datastructures is placed in the on-error section, which is skipped
under normal circumstances where it succeeds for that service. Relevant
snippet, which comes right after the completion of the per-service code
in the loop in do_v5_kvno:

329 continue;
330
331 error:
332 if (server != NULL)
333 krb5_free_principal(context, server);
334 if (ticket != NULL)
335 krb5_free_ticket(context, ticket);
336 if (out_creds != NULL)
337 krb5_free_creds(context, out_creds);
338 if (princ != NULL)
339 krb5_free_unparsed_name(context, princ);
340 errors++;
341 }

Impact:

No significant impact. kvno is short-lived and cleans up on exit, but
it seems worth avoiding memory leaks anyway.

Sample patch to fix this problem:

--- kvno.c 2017-03-02 17:06:02.000000000 -0500
+++ kvno.c 2017-03-11 15:45:39.876194069 -0500
@@ -326,6 +326,15 @@
printf(_("%s: kvno = %d\n"), princ,
ticket->enc_part.kvno); }

+ if (server != NULL)
+ krb5_free_principal(context, server);
+ if (ticket != NULL)
+ krb5_free_ticket(context, ticket);
+ if (out_creds != NULL)
+ krb5_free_creds(context, out_creds);
+ if (princ != NULL)
+ krb5_free_unparsed_name(context, princ);
+
continue;

error:

No copyright claimed on patch for obvious reasons, but there's probably
a more elegant solution that avoids code duplication anyway.

Cheers,
Cel Skeggs.
From: ghudson@mit.edu
Subject: git commit

Fix minor memory leaks in kvno

In do_k5_kvno(), free allocated values on success as well as failure.
In t_kdb.py, run kvno with multiple arguments to manifest this leak in
asan and valgrind. Reported by Cel Skeggs.

https://github.com/krb5/krb5/commit/16d3eaf055a2e487ca28fa71e1e57802bc47310d
Author: Greg Hudson <ghudson@mit.edu>
Commit: 16d3eaf055a2e487ca28fa71e1e57802bc47310d
Branch: master
src/clients/kvno/kvno.c | 8 +++++---
src/tests/t_kdb.py | 3 +--
2 files changed, 6 insertions(+), 5 deletions(-)
Thanks for the detailed bug report. In addition to the minimal fix
referenced in the ticket, I also (in commit
6d82212636eec154bb7322ce7a6e02fd6d1aa596) moved the loop body into a
helper function so that its memory management could conform to our
normal pattern.
Date: Mon, 13 Mar 2017 13:15:19 -0400
From: Cel Skeggs <cela@mit.edu>
To: <rt-comment@krbdev.mit.edu>
Subject: Re: [krbdev.mit.edu #8558] kvno memory leak (1.15.1)
RT-Send-Cc:
Excellent. Thanks for taking care of this!

On Mon, 13 Mar 2017 13:04:40 -0400
Greg Hudson via RT <rt-comment@krbdev.mit.edu> wrote:

Show quoted text
> Thanks for the detailed bug report. In addition to the minimal fix
> referenced in the ticket, I also (in commit
> 6d82212636eec154bb7322ce7a6e02fd6d1aa596) moved the loop body into a
> helper function so that its memory management could conform to our
> normal pattern.