Bug 1379482

Description of problem:

While looking into multiple memory leaks, I came across this.  It may
or not be the cause of the particular leak I'm currently looking
into, but it could be, and it's certainly a bug.  In /src/gp_creds.c,
in gp_get_cred_environment, we have:

[...]
    338     for (d = 0; d < svc->krb5.cred_count; d++) {
    339         p = strchr(svc->krb5.cred_store[d], ':');
    340         if (!p) {
    341             GPERROR("Invalid cred_store value"
    342                     "no ':' separator found in [%s].\n",
    343                     svc->krb5.cred_store[d]);
    344             ret = EINVAL;
    345             goto done;
    346         }
    347 
    348         if (strncmp(svc->krb5.cred_store[d], "client_keytab:", 14) == 0) {
    349             ck_num = c;
    350         } else if (strncmp(svc->krb5.cred_store[d], "keytab:", 7) == 0) {
    351             k_num = c;
    352         }
    353 
    354         ret = asprintf(&str, "%.*s", (int)(p - svc->krb5.cred_store[d]),
    355                                      svc->krb5.cred_store[d]);
    356         if (ret == -1) {
    357             ret = ENOMEM;
    358             goto done;
    359         }
    360         cs->elements[c].key = str;
    361 
    362         fmtstr = p + 1;
    363         cs->elements[c].value = get_formatted_string(fmtstr, target_uid);
    364         if (!cs->elements[c].value) {
    365             GPDEBUG("Failed to build credential store formatted string.\n");
    366             ret = ENOMEM;
    367             goto done;
    368         }
    369 
    370         c++;
    371     }
    372     cs->count = c;
[...]

"done" looks like:

[...]
    410 done:
    411     if (ret) {
    412         free_cred_store_elements(cs);
    413     }
    414     return ret;
    415 }
[...]

free_cred_store_elements looks like:

[...]
    206 static void free_cred_store_elements(gss_key_value_set_desc *cs)
    207 {
    208     int i;
    209 
    210     for (i = 0; i < cs->count; i++) {
    211         safefree(cs->elements[i].key);
    212         safefree(cs->elements[i].value);
    213     }
    214     safefree(cs->elements);
    215 }
[...]

safefree looks like:

     60 #define safefree(ptr) do { \
     61     free(no_const(ptr)); \
     62     ptr = NULL; \
     63 } while(0)

OK, cs->elements does get calloc(3)ed earlier in the
gp_get_cred_environment function.  For now, I'm going to assume that
cs->count comes into this function initialized to 0.  If it doesn't
I'll file another bug for that because other stuff can break causing
segfaults and whatnot if that's not the case.  Anyway, for now, we'll
just assume it's initialized properly and deal with that separately
later if that turns out not to be the case.

So, I can see what you're trying to do in that loop.  You're trying
to only fill in elements of the cs->elements array that matter.  So,
rather than having cs->count track the index from the source (the "d"
loop counter), we'll just keep track of how many elements we update in
the "c" temporary variable, and at the end of the loop, we'll update
cs->count to equal the number of elements we actually put in there.
Unfortunately, there are several places in that loop where we could
error out, and exit the function via the "done" label which should
free up everything in cs->elements, then free cs->elements itself.
However, if we ever do actually hit one of those conditions in that
loop, we never set cs->count, so it's either some random number
if not initialized properly in whatever created it (I'll file a
separate bug later if that turns out to be the case), or if it is
initialized properly to 0, well, none of the "cs->elements[i].key"
"cs->elements[i].value" memory will be freed.  Either way, it's a bug.
Given that valgrind had this:

==24113== 259,728 (148,416 direct, 111,312 indirect) bytes in 4,638 blocks are definitely lost in loss record 82 of 85
==24113==    at 0x4C2B974: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24113==    by 0x408063: gp_get_cred_environment (gp_creds.c:331)
==24113==    by 0x408063: gp_add_krb5_creds (gp_creds.c:461)
==24113==    by 0x40DD15: gp_accept_sec_context (gp_rpc_accept_sec_context.c:79)
==24113==    by 0x40ADC0: gp_rpc_execute (gp_rpc_process.c:343)
==24113==    by 0x40ADC0: gp_rpc_process_call (gp_rpc_process.c:400)
==24113==    by 0x4073CB: gp_handle_query (gp_workers.c:447)
==24113==    by 0x4073CB: gp_worker_main (gp_workers.c:401)
==24113==    by 0x6822DC4: start_thread (pthread_create.c:308)
==24113==    by 0x6B2DCEC: clone (clone.S:113)

to say about the leak I'm currently tracking down, I'd say this could
easily be at least part of the problem, because _not_ freeing a bunch
of ...value and ...key pairs , but actually freeing the ...elements
array that contains pointers to those two things would generate a
bunch of indirect leaks, and I think it would show up in the loss record
indicated because it's actually the freeing of ...elements that causes
the indirect leak, and the code stack indicated would be what allocates
that ...elements memory.  I think that's the way it would work....

Anyway, it's not the whole problem, for certain, because I don't
think this would cause any of the direct leaks, but it could certainly
cause indirect ones, and it's certainly a bug.

The good news is that I think it's probably pretty easy to fix.
The way the code currently stands, I think just changing the
initialization of the "c" temporary variable to 0 to up where it's
defined at the top of the function (instead of just before the loop),
and then changing the "done" code to:

done:
    if (!cs->count) cs->count = c;
    if (ret) {
        free_cred_store_elements(cs);
    }
    return ret;

should probably cover it without making anything any worse.  Verify that
for yourself, though.  That was the first thing to pop into my wee-tiny
little brain, and it "looks" right to me, but....

Version-Release number of selected component (if applicable):

gssproxy-0.4.1-13.el7

How reproducible:

Every time.  Just look at the code, and there it will be until someone
changes it!  :-)

Steps to Reproduce:
1. Look at the code.
2. See the bug.
3. Slap self in forehead.  Pounding head on desk, weeping and wailing
are all optional, of course.

Actual results:

Indirect memory leak.

Expected results:

Memory not leaked, indirectly or otherwisely.

Additional info: