RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 985005 - p11-kit: memory leaks on error paths
Summary: p11-kit: memory leaks on error paths
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: p11-kit
Version: 6.5
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Stef Walter
QA Contact: BaseOS QE Security Team
URL:
Whiteboard:
Depends On:
Blocks: 983512
TreeView+ depends on / blocked
 
Reported: 2013-07-16 14:38 UTC by Florian Weimer
Modified: 2013-07-16 20:08 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-07-16 15:06:07 UTC
Target Upstream Version:
Embargoed:


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

Description Florian Weimer 2013-07-16 14:38:26 UTC
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 15:06:07 UTC
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 15:13:51 UTC
(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 15:25:52 UTC
(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 16:25:32 UTC
(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 16:34:28 UTC
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 16:45:17 UTC
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.