dhcp: Free unused unknown dict attrs #795

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@spbnick
Collaborator
spbnick commented Sep 17, 2014

Free unused unknown dict attrs not attached to value pairs in
fr_dhcp_decode_suboption.

Coverity has reported the following error for this issue.

Error: RESOURCE_LEAK (CWE-772):
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:731: alloc_fn: Storage is returned from allocation function "dict_attrunknown(unsigned int, unsigned int, int)".
freeradius-server-3.0.4/src/lib/dict.c:2666:2: alloc_fn: Storage is returned from allocation function "malloc(size_t)".
freeradius-server-3.0.4/src/lib/dict.c:2666:2: var_assign: Assigning: "da" = "malloc(148UL)".
freeradius-server-3.0.4/src/lib/dict.c:2671:2: noescape: Resource "da" is not freed or pointed-to in function "memset(void _, int, size_t)".
freeradius-server-3.0.4/src/lib/dict.c:2687:2: var_assign: Assigning: "p" = "da".
freeradius-server-3.0.4/src/lib/dict.c:2689:2: noescape: Resource "p" is not freed or pointed-to in function "snprintf(char * restrict, size_t, char const * restrict, ...)".
freeradius-server-3.0.4/src/lib/dict.c:2712:3: noescape: Resource "p" is not freed or pointed-to in function "snprintf(char * restrict, size_t, char const * restrict, ...)".
freeradius-server-3.0.4/src/lib/dict.c:2718:2: noescape: Resource "p" is not freed or pointed-to in function "print_attr_oid(char *, size_t, unsigned int, int)".
freeradius-server-3.0.4/src/lib/dict.c:2546:36: noescape: "print_attr_oid(char *, size_t, unsigned int, int)" does not free or save its pointer parameter "buffer".
freeradius-server-3.0.4/src/lib/dict.c:2720:2: return_alloc: Returning allocated memory "da".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:731: var_assign: Assigning: "da" = storage returned from "dict_attrunknown(attr, (_tlv)->da->vendor, 1)".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:740: noescape: Resource "da" is not freed or pointed-to in function "fr_dhcp_array_members(size_t *, DICT_ATTR const *)".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:467:64: noescape: "fr_dhcp_array_members(size_t *, DICT_ATTR const *)" does not free or save its pointer parameter "da".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:758: leaked_storage: Variable "da" going out of scope leaks the storage it points to.

@spbnick spbnick dhcp: Free unused unknown dict attrs
Free unused unknown dict attrs not attached to value pairs in
fr_dhcp_decode_suboption.

Coverity has reported the following error for this issue.

Error: RESOURCE_LEAK (CWE-772):
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:731: alloc_fn: Storage is returned from allocation function "dict_attrunknown(unsigned int, unsigned int, int)".
freeradius-server-3.0.4/src/lib/dict.c:2666:2: alloc_fn: Storage is returned from allocation function "malloc(size_t)".
freeradius-server-3.0.4/src/lib/dict.c:2666:2: var_assign: Assigning: "da" = "malloc(148UL)".
freeradius-server-3.0.4/src/lib/dict.c:2671:2: noescape: Resource "da" is not freed or pointed-to in function "memset(void *, int, size_t)".
freeradius-server-3.0.4/src/lib/dict.c:2687:2: var_assign: Assigning: "p" = "da".
freeradius-server-3.0.4/src/lib/dict.c:2689:2: noescape: Resource "p" is not freed or pointed-to in function "snprintf(char * restrict, size_t, char const * restrict, ...)".
freeradius-server-3.0.4/src/lib/dict.c:2712:3: noescape: Resource "p" is not freed or pointed-to in function "snprintf(char * restrict, size_t, char const * restrict, ...)".
freeradius-server-3.0.4/src/lib/dict.c:2718:2: noescape: Resource "p" is not freed or pointed-to in function "print_attr_oid(char *, size_t, unsigned int, int)".
freeradius-server-3.0.4/src/lib/dict.c:2546:36: noescape: "print_attr_oid(char *, size_t, unsigned int, int)" does not free or save its pointer parameter "buffer".
freeradius-server-3.0.4/src/lib/dict.c:2720:2: return_alloc: Returning allocated memory "da".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:731: var_assign: Assigning: "da" = storage returned from "dict_attrunknown(attr, (*tlv)->da->vendor, 1)".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:740: noescape: Resource "da" is not freed or pointed-to in function "fr_dhcp_array_members(size_t *, DICT_ATTR const *)".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:467:64: noescape: "fr_dhcp_array_members(size_t *, DICT_ATTR const *)" does not free or save its pointer parameter "da".
freeradius-server-3.0.4/src/modules/proto_dhcp/dhcp.c:758: leaked_storage: Variable "da" going out of scope leaks the storage it points to.
e088a87
@alandekok
Member

The issue is larger than that. If the da is allocated dynamically, then pairalloc(ctx, da) will add a reference from the vp to the da. And pairfree() will call dict_attr_free(..) on the da.

Why is this a problem?

num_entries = fr_dhcp_array_members(&a_len, da);
for (i = 0; i < num_entries; i++) {
    vp = pairalloc(ctx, da);

So the da will be referenced from multiple vps, and the da will be freed multiple times.

Your patch doesn't work because it always frees da, even the da is still in use by a vp.

The best solution would be to modify dict_attrunknown() to use talloc. All of the problems will then go away.

@alandekok alandekok added a commit that referenced this pull request Sep 17, 2014
@alandekok alandekok Free unknown DICT_ATTRs. Closes #795
The unknown DICT_ATTRs are converted to talloc, and various
sweeping changes made through the code to accomodate this.
76f0ab3
@alandekok alandekok added a commit that closed this pull request Sep 17, 2014
@alandekok alandekok Free unknown DICT_ATTRs. Closes #795
The unknown DICT_ATTRs are converted to talloc, and various
sweeping changes made through the code to accomodate this.
6d7238e
@alandekok alandekok closed this in 6d7238e Sep 17, 2014
@spbnick
Collaborator
spbnick commented Sep 17, 2014

Thanks for a quick response, Alan.

I admit I haven't tested this patch and was hoping to rely on your judgement.

Yes, I suspected there was a problem with da being attached to multiple value pairs.
However, it manages to work somehow without my fix and that multiple attachment will certainly not result in a memory leak. I've seen the comment about the need to move da's to talloc.

Yet, I thought that fixing that memory leak might be useful, since it seems that num_entries comes from the contents of DHCP packet coming from outside. And it's theoretically possible that a flood of packets with num_entries == 0 could result in a considerable leak. It is just my guess, though.

Regarding the patch always freeing the da - doesn't it handle that with the i == 0 condition? I.e. doesn't it free the da only if it wasn't attached to any vp yet?

@spbnick
Collaborator
spbnick commented Sep 17, 2014

Ah, I see that you merged a proper fix to v3.0.x. Great! Thank you.

@pwdng pwdng added a commit to pwdng/freeradius-server that referenced this pull request Oct 24, 2014
@alandekok @pwdng alandekok + pwdng Free unknown DICT_ATTRs. Closes #795
The unknown DICT_ATTRs are converted to talloc, and various
sweeping changes made through the code to accomodate this.
55812ec
@pwdng pwdng added a commit to pwdng/freeradius-server that referenced this pull request Oct 24, 2014
@alandekok @pwdng alandekok + pwdng Free dynamically allocated DA. Fixes #795 9d51529
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment