Red Hat Bugzilla – Bug 985005
p11-kit: memory leaks on error paths
Last modified: 2013-07-16 16:08:00 EDT
tools/extract-info.c:p11_extract_info_comment() leaks memory pointed to by the label variable on asprintf error. trust/index.c:p11_index_new() leaks the index pointer allocated by calloc() if p11_dict_new() or the second calloc() fails. Similarly for p11_dict_new(). trust/index.c:bucket_insert() leaks memory on realloc() error by direct assignment of the result to the pointer being realloc'ed. Same in bucket_push(). trust/token.c:p11_token_new() leaks memory on allocation failure (multiple cases). p11-kit/iter.c:p11_kit_iter_new() leaks the pointer returned by calloc() if p11_array_new() fails. There are likely more such cases.
We do not treat memory allocation failures as a valid code or error path. For an explanation please see: http://p11-glue.freedesktop.org/doc/p11-kit/devel-building-style.html (In reply to Florian Weimer from comment #0) > tools/extract-info.c:p11_extract_info_comment() leaks memory pointed to by > the label variable on asprintf error. > > trust/index.c:p11_index_new() leaks the index pointer allocated by calloc() > if p11_dict_new() or the second calloc() fails. Similarly for > p11_dict_new(). > > trust/index.c:bucket_insert() leaks memory on realloc() error by direct > assignment of the result to the pointer being realloc'ed. Same in > bucket_push(). > > trust/token.c:p11_token_new() leaks memory on allocation failure (multiple > cases). > > p11-kit/iter.c:p11_kit_iter_new() leaks the pointer returned by calloc() if > p11_array_new() fails. > > There are likely more such cases. Indeed. We treat memory allocation as a precondition, as we do valid input, and so on. And we warn and return immediately without cleanup in the presence of failed precondition. You can see preconditions being checked via the following function: p11_debug_precond() and the macros which call into it: return_val_if_fail() return_if_fail() return_if_reached() return_val_if_reached() warn_if_reached() warn_if_fail() I've reviewed each of these cases. Thank you for the research digging them out, but they appear to be false positives. Please reopen this bug if you feel I've misunderstood things.
(In reply to Stef Walter from comment #2) > Indeed. We treat memory allocation as a precondition, as we do valid input, > and so on. What does this actually mean? If memory allocation fails, the caller violated the precondition, and so behavior is undefined?
(In reply to Florian Weimer from comment #4) > (In reply to Stef Walter from comment #2) > > > Indeed. We treat memory allocation as a precondition, as we do valid input, > > and so on. > > What does this actually mean? If memory allocation fails, the caller > violated the precondition, and so behavior is undefined? Yes, the system or caller violated the precondition. And yes behavior is undefined. These are not recoverable errors, although since we are a library, we do try to bail in a mostly debuggable rather than just call abort(). When a precondition fails it is a bug. Whether a bug in the system, or a bug in the caller, or a bug in p11-kit (although we usually try to use assertions for stronger guarantees within a single unit of p11-kit code). You can use P11_KIT_STRICT=1 to just abort() when a precondition fails. We do this during testing, for example.
(In reply to Stef Walter from comment #6) > (In reply to Florian Weimer from comment #4) > > (In reply to Stef Walter from comment #2) > > > > > Indeed. We treat memory allocation as a precondition, as we do valid input, > > > and so on. > > > > What does this actually mean? If memory allocation fails, the caller > > violated the precondition, and so behavior is undefined? > > Yes, the system or caller violated the precondition. And yes behavior is > undefined. These are not recoverable errors, although since we are a > library, we do try to bail in a mostly debuggable rather than just call > abort(). Hmm. I'm not sure if this is the right approach for a fairly central system library. But I could be wrong about that, and this bug is clearly not the place to discuss that.
Agreed there are probably better places to discuss this. Although the general approach has been discussed over and over in many places. FWIW, although this seems awkward, i believe it is an approach to far more robust software. It avoids filling code with completely unreacheable error paths which are never tested or run (eg: recovering memory allocation failures). Any glib based library makes much stronger choices, such as aborting on memory allocation failure. In addition they have the precondition on invalid input as you see here.
Created attachment 774408 [details] Make prconditions abort unconditionally when scanning with coverity Does this patch look like it'll do the trick of making coverity understand the p11-kit handling of irreparable problems