PR: https://github.com/SSSD/sssd/pull/968

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

Commit 26e33b1 fixes this issue

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

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.

PR: https://github.com/SSSD/sssd/pull/977

Commit 0bb1289 fixes this issue

• 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

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/5096

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.