Coverity complains of a use-after-free in source3/libads/ldap_schema.c:ads_get_attrname_by_guild

[Jeremy Allison]

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.

Jeremy.