#4135 util/sss_ptr_hash.c: potential double free in `sss_ptr_hash_delete_cb()`
Closed: Fixed 10 days ago by pbrezina. Opened 2 months ago by atikhonov.

Calling data->callback(value->ptr) in sss_ptr_hash_delete_cb() can lead to freeing of value->ptr and thus to destruction of value->spy (that is attached to value->ptr).
In turn sss_ptr_hash_spy_destructor() calls sss_ptr_hash_delete() -> hash_delete() -> sss_ptr_hash_delete_cb() again and in this recursive execution hash entry is actually deleted and value is freed.
When stack unwinds back to "first" sss_ptr_hash_delete_cb() it tries to free value again => double free.

This bug can actually be seen in https://bugzilla.redhat.com/show_bug.cgi?id=1783190

Easy solution is to disable spy destructor in this execution chain. Another option could be to get rid of spy at all.


Metadata Update from @atikhonov:
- Custom field rhbz adjusted to https://bugzilla.redhat.com/show_bug.cgi?id=1783190

2 months ago

Metadata Update from @atikhonov:
- Issue tagged with: PR

2 months ago

Bug was introduced in f95db37 and doesn't affect 1-16 branch.

  • master
    • 26e33b1 - util/sss_ptr_hash: fixed double free in sss_ptr_hash_delete_cb()

Metadata Update from @atikhonov:
- Custom field design_review adjusted to on
- Custom field mark adjusted to on
- Custom field patch adjusted to on
- Custom field review adjusted to on
- Custom field sensitive adjusted to on
- Custom field testsupdated adjusted to on
- Issue status updated to: Open (was: Closed)

a month ago

Well, while fixing "double free" (introduced in f95db37) I have re-introduced "use after free" ("accidentally" fixed by the same f95db37): https://bugzilla.redhat.com/show_bug.cgi?id=1792331

    value = talloc_get_type(item->value.ptr, struct sss_ptr_hash_value);
    if (value == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "Invalid value!\n");
        return;
    }

    callback_entry.key = item->key;
    callback_entry.value.type = HASH_VALUE_PTR;
    callback_entry.value.ptr = value->ptr;

    /* Free value, this also will disable spy */
    talloc_free(value);

    /* Switch to the input value and call custom callback. */
    if (data->callback != NULL) {
        data->callback(&callback_entry, deltype, data->pvt);
    }

I guess we should postpone freeing the value after callback is called. But we should mark it as deleted so it is not possible to look it up/delete it again. IMHO this is should have been done in dhash.

I guess we should postpone freeing the value after callback is called. But we should mark it as deleted so it is not possible to look it up/delete it again. IMHO this is should have been done in dhash.

So you think https://github.com/alexey-tikhonov/sssd/commit/e92088b4bb8a29dafeaa7c6afab55748bd81485a would not be enough?

This solves the problem if someone calles talloc_free(value->ptr), i.e. when someone frees to stored value. However the key and internal sss_ptr_value can still be looked up and deleted again with sss_ptr_hash_lookup and sss_ptr_hash_delete. Once we get into delete callback we should treated the sss_ptr_value as removed from the hash table so it can not be accessed again.

However the key and internal sss_ptr_value can still be looked up

Right, this actually happens in sbus-server related code. And my understanding was that this was "by design".

and deleted again with sss_ptr_hash_lookup and sss_ptr_hash_delete.

IIUC, this doesn't happen currently. Do you mean there is potential for this?

Once we get into delete callback we should treated the sss_ptr_value as removed from the hash table so it can not be accessed again.

I think I do not like an idea to update libdhash and complicate things further...

However the key and internal sss_ptr_value can still be looked up

Right, this actually happens in sbus-server related code. And my understanding was that this was "by design".

Right. I need to spend some time debugging it if it is needed or it is used as a workaround for the fact that the name is still available in the table.

and deleted again with sss_ptr_hash_lookup and sss_ptr_hash_delete.

IIUC, this doesn't happen currently. Do you mean there is potential for this?

It is being looked up again in the sbus code as you said. But it is there to actually not send signal to the connection with this name so perhaps it can be avoided.

Once we get into delete callback we should treated the sss_ptr_value as removed from the hash table so it can not be accessed again.

I think I do not like an idea to update libdhash and complicate things further...

I agree.

  • master
    • 88b23bf - TESTS: added sss_ptr_hash unit test
    • 0bb1289 - sss_ptr_hash: internal refactoring
    • 4bc0c2c - sss_ptr_hash: fixed memory leak
    • 8cc2ce4 - sss_ptr_hash: removed redundant check
    • d0eb880 - sss_ptr_hash: sss_ptr_hash_delete fix/optimization
    • adc7730 - sss_ptr_hash: don't keep empty sss_ptr_hash_delete_data
    • faa5dbf - sbus_server: stylistic rename

Metadata Update from @pbrezina:
- Issue status updated to: Open (was: Closed)

10 days ago

Metadata Update from @pbrezina:
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)

10 days ago

Login to comment on this ticket.

Metadata