Coverity complains of a use-after-free in source3/libads/ldap_schema.c:ads_get_attrname_by_guild
Richard Sharpe
realrichardsharpe at gmail.com
Fri Oct 18 17:05:56 MDT 2013
On Fri, Oct 18, 2013 at 3:55 PM, Jeremy Allison <jra at samba.org> wrote:
> On Fri, Oct 18, 2013 at 12:06:46PM -0700, Richard Sharpe wrote:
>> Hi folks,
>>
>> At line 141 we see:
>>
>> rc = ads_do_search_retry(ads, schema_path, LDAP_SCOPE_SUBTREE,
>> expr, attrs, &res);
>> if (!ADS_ERR_OK(rc)) {
>> goto done;
>> }
>>
>> if (ads_count_replies(ads, res) != 1) {
>> goto done;
>> }
>>
>> result = ads_pull_string(ads, mem_ctx, res, "lDAPDisplayName");
>>
>> done:
>> TALLOC_FREE(guid_bin);
>> ads_msgfree(ads, res);
>> return result;
>>
>> }
>>
>>
>> However, ads_do_search_retry will free the object pointed to by ads
>> when an error occurs, it seems. Then after done: we call ads_msgfree
>> and pass ads.
>>
>> Looks like a use after free to me.
>
> Actually it's safe. Look at ads_msgfree().
>
> void ads_msgfree(ADS_STRUCT *ads, LDAPMessage *msg)
> {
> if (!msg) return;
> ldap_msgfree(msg);
> }
>
> Why we pass the ads struct is a bit of a mystery to me :-).
>
> In the codepath where we return with ads_destroy(&ads);
> we have already done:
>
> if (*res)
> ads_msgfree(ads, *res);
> *res = NULL;
>
> so we've freed msg also. This is a *horrible* interface,
> but it isn't a use after free in this case.
OK, great. One more false positive.
--
Regards,
Richard Sharpe
(何以解憂?唯有杜康。--曹操)
More information about the samba-technical
mailing list