Add a free_e_data function pointer to krb5_db_entry

[cryptomilk]

There are projects out there which provide kdb modules and use a
different memory allocator than malloc. One example is Samba which uses
talloc. Those projects should to be able to free e_data using their
memory allocation functions.

[cryptomilk]

I would need this in 1.15. Currently we allocate memory till the OOM shows up. Implementing it differently would be a bad for performance.

[cryptomilk]

It would also be fine to restore the free_principal in the kdb vtable and make it optional.

Then the modules which need it could implement it freeing e_data first and then calling krb5_db_free_principal().

[greghudson]

It's not great that the 1.15 changes make life harder for Samba, but 1.15 is released and I'm not sure we can safely make a change of this nature.

How bad for performance is it to use malloc() or krb5_db_alloc() for the e_data pointer? What do you mean by "Currently we allocate memory till the OOM shows up"?

[abbra]

@greghudson Samba uses its own hierarchical allocator, talloc, which allows to free all memory associated with a context in one go. If krb5 1.15 frees e_data allocated with talloc with conventional free(), then all child allocations of that context are lost. This is what @cryptomilk means "currently we allocate memory till the OOM shows up" as for the lifetime of the krb5kdc/kadmind the child allocations will stay there.

Samba tries to maintain common abstraction for both Heimdal and MIT krb5 backends. In HDB there is a callback to free the entry, set in the entry itself, like @cryptomilk proposes in this patch. Samba's callback handles talloc-based deallocation there.

Ideally, a revert of the commit that went into 1.15 would be welcomed. If that is not possible, adding a per-entry free callback is an option.

[cryptomilk]

We do not need to fully revert it. A an optional free_principal callback in the kdb vtable would be fine. Then you could create function like this:

void free_principal(krb5_context ctx, krb5_db_entry *e)
{
   talloc_free(e->e_data);
   e->e_data = NULL;
   krb5_db_free_principal(e);
}

If you prefer this over the function I suggested, I could code this up and document it.

Andreas

[greghudson]

I think I understand. e_data points to a multi-part data structure, and the only way to arrange for it to be freed correctly with the new KDB interface would be to marshal it into a single malloc()'d memory region with interior pointers pointing into later parts of the memory buffer.

I will think more about whether the change in this PR is safe for 1.15 or not.

[greghudson]

After talking about this issue with Simo and others, I think the conservative fix for 1.15 is to add a DAL function to free the principal e_data pointer (not the whole principal) and increment the minor version. That's probably also a reasonable solution going forward, although it's different from Heimdal's design.

Unfortunately, I don't think we have ever used the kdb_vftabl min_ver field in the past, and kdb5.c:kdb_load_library() isn't set up to use it. I don't think it's reasonable to ask Andreas to figure this out, so I will put it on my short-term todo list.

[cryptomilk]

We add a free_principal_e_data function pointer to the vtable to allow to free e_data and call it before krb5_db_free_principal() if e_data is not NULL?

I could code up that part if you want me to.

[greghudson]

That's the general idea, yes.

[cryptomilk]

Here we go.

[greghudson]

I pushed updated commits. Please test.

(I do not want to expose a KRB5_KDB_DAL_MINOR_VERSION macro. If modules use that in their vtables, we could treat their vtables as having been updated for a minor version bump when they haven't been.)

[cryptomilk]

Removed unused variable status. Testing with Samba now ...

[cryptomilk]

Ok, Samba AD is back in a working state with these patches.