Bug 985005 - p11-kit: memory leaks on error paths
p11-kit: memory leaks on error paths
Status: CLOSED NOTABUG
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: p11-kit (Show other bugs)
6.5
Unspecified Unspecified
unspecified Severity unspecified
: rc
: ---
Assigned To: Stef Walter
BaseOS QE Security Team
:
Depends On:
Blocks: 983512
  Show dependency treegraph
 
Reported: 2013-07-16 10:38 EDT by Florian Weimer
Modified: 2013-07-16 16:08 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-07-16 11:06:07 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Make prconditions abort unconditionally when scanning with coverity (1.33 KB, patch)
2013-07-16 12:45 EDT, Stef Walter
no flags Details | Diff

  None (edit)
Description Florian Weimer 2013-07-16 10:38:26 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.
Comment 2 Stef Walter 2013-07-16 11:06:07 EDT
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.
Comment 4 Florian Weimer 2013-07-16 11:13:51 EDT
(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?
Comment 6 Stef Walter 2013-07-16 11:25:52 EDT
(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.
Comment 9 Florian Weimer 2013-07-16 12:25:32 EDT
(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.
Comment 10 Stef Walter 2013-07-16 12:34:28 EDT
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.
Comment 11 Stef Walter 2013-07-16 12:45:17 EDT
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

Note You need to log in before you can comment on or make changes to this bug.