(In reply to comment #0)
> Running libisds/curl/gnutls/p11-kit stack under valgrind shows this memory
> leak warning:
> 
> ==1893== 1 bytes in 1 blocks are still reachable in loss record 1 of 78
> ==1893==    at 0x4C2C88D: malloc (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==1893==    by 0x6F9B1F9: strdup (in /lib64/libc-2.15.so)
> ==1893==    by 0x7B7D9A9: p11_kit_pin_register_callback (in
> /usr/lib64/libp11-kit.so.0.0.0)
> ==1893==    by 0x5F7C792: gnutls_pkcs11_init (in
> /usr/lib64/libgnutls.so.26.22.4)
> ==1893==    by 0x5F66C2A: gnutls_global_init (in
> /usr/lib64/libgnutls.so.26.22.4)
> ==1893==    by 0x56BD7D6: Curl_gtls_init (in /usr/lib64/libcurl.so.4.2.0)
> ==1893==    by 0x56B2049: curl_global_init (in /usr/lib64/libcurl.so.4.2.0)
> ==1893==    by 0x406EDA: isds_init (isds.c:643)
> ==1893==    by 0x4037BB: main (context.c:34)
> 
> And indeed current p11-kit p11_kit_pin_register_callback() calls
> register_callback_unlocked (pin_source, cb) which has this code:
> 
> {
>     ptr_array_t *callbacks = NULL;
>     char *name;
> 
>     name = strdup (pin_source);
> ! Allocation ^^^
>     return_val_if_fail (name != NULL, -1);
> 
>     if (gl.pin_sources == NULL) {
>         gl.pin_sources = _p11_hash_create (_p11_hash_string_hash,
> _p11_hash_string_equal,
>                                            free,
> (hash_destroy_func)_p11_ptr_array_free);
> ! Here you forgot to free(name).
>         return_val_if_fail (gl.pin_sources != NULL, -1);

Nope, this is intentional. See HACKING.

>     }
> 
>     if (gl.pin_sources != NULL)
>         callbacks = _p11_hash_get (gl.pin_sources, name);
> ! Here _p11_hash_get() does not store name, it's Ok.
> 
>     if (callbacks == NULL) {
>         callbacks = _p11_ptr_array_create (unref_pin_callback);
> ! Here you forgot to free(name).
>         return_val_if_fail (callbacks != NULL, -1);

Nope, internional. See HACKING again.

>         if (!_p11_hash_set (gl.pin_sources, name, callbacks))
> ! This is very hard place. _p11_hash_set() stores name if it does not exist
> in hashmap. Otherwise it does not store it. However it does so regardeless
> the return value. The API is poor here. It's not possible to guard the
> memory.

Hmmm. good catch. You would patch _p11_hash_set() so that it free's the old key (if there is a key_destroy_func), and replaces it with the new one.

Are you interested in doing a patch for this?

> The first two places are easy to fix. The third one requieres changes in
> hashmap API.

I don't think it requires a change in API, see above. Could you explain why that's necessary?

Created attachment 72612 [details] [review]
I decided to put together a patch. Does the attached patch fix the leak for you?

I gave a try and valgrind output is still the same (except memory pointers). Maybe the leak reported by valgrind is somewhere else and I just found random flaw when reading code around the place. Maybe gnutls does use p11-kit in wrong way and it's one of the HACKING-approach leak.

By the way, the HACKING file is not in tar ball. It's in git tree only.

I meant with API change to track storing. Replaceing the key everytime is also solution.

Feel free to close this report, if you are happy with it. I'm not in mood for debugging it now. I see you wrote a test, so I'm glad at least the one leak has been fixed.

(In reply to comment #3)
> I gave a try and valgrind output is still the same (except memory pointers).
> Maybe the leak reported by valgrind is somewhere else and I just found
> random flaw when reading code around the place. Maybe gnutls does use
> p11-kit in wrong way and it's one of the HACKING-approach leak.

There will always be a message printed out when an unexpected precondition is violated. So it's pretty clear if this is the case.

> By the way, the HACKING file is not in tar ball. It's in git tree only.

Hmm, good point. I'll fix that.

> I meant with API change to track storing. Replaceing the key everytime is
> also solution.

Ah ok.

> Feel free to close this report, if you are happy with it. I'm not in mood
> for debugging it now. I see you wrote a test, so I'm glad at least the one
> leak has been fixed.

Thanks for catching this, and for your time. And if you find any other leaks or problems please do file bugs.