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