[krbdev.mit.edu #8558] kvno memory leak (1.15.1)

[Cel Skeggs via RT]

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.