[sssd PR#930][opened] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Author: alexey-tikhonov
 Title: #930: SBUS: remove dbus fd from tevent loop when watch is removed
Action: opened

PR body:
"""
According to the tevent API:
"To cancel the monitoring of a file descriptor, call talloc_free()
on the object returned by this (tevent_add_fd()) function."

Resolves: https://pagure.io/SSSD/sssd/issue/2660
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/930/head:pr930
git checkout pr930

[sssd PR#930][+Waiting for review] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: SBUS: remove dbus fd from tevent loop when watch is removed

Label: +Waiting for review

[sssd PR#930][comment] SBUS: remove dbus fd from tevent loop when watch is removed

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: SBUS: remove dbus fd from tevent loop when watch is removed

pbrezina commented:
"""
Patch from sssd-2.x: f1f9af528f

The interesting thing is that `watch->fde` should be freed with `watch` because it is its parent (see the code below) and there should not be any need to free it explicitly. I fixed it in sssd-2.x because valgrind kept complaining about it, but I do not recall any crash caused by it.

```
    watch->fde = tevent_add_fd(conn->ev,
                               watch, fd, event_flags,
                               sbus_watch_handler, watch);
```

But at least the patch is harmless so we can push it with hope that it will fix the issue... can you perhaps try to catch it with valgrind?

"""

See the full comment at https://github.com/SSSD/sssd/pull/930#issuecomment-550230440

[sssd PR#930][-Waiting for review] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: SBUS: remove dbus fd from tevent loop when watch is removed

Label: -Waiting for review

[sssd PR#930][comment] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: SBUS: remove dbus fd from tevent loop when watch is removed

alexey-tikhonov commented:
"""
> Patch from sssd-2.x: [f1f9af5](https://github.com/SSSD/sssd/commit/f1f9af528f71f42ac41bb7a272f4f7d940fd3a0f)

Thank you for the link!
> The interesting thing is that `watch->fde` should be freed with `watch` because it is its parent (see the code below) and there should not be any need to free it explicitly.

Yeap, that is exactly the thing that came up into my mind this night. I need to check the details.

"""

See the full comment at https://github.com/SSSD/sssd/pull/930#issuecomment-550235281

[sssd PR#930][edited] [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Author: alexey-tikhonov
 Title: #930: [WiP] SBUS: remove dbus fd from tevent loop when watch is removed
Action: edited

Changed field: title
Original value:
"""
SBUS: remove dbus fd from tevent loop when watch is removed
"""

[sssd PR#930][comment] [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

alexey-tikhonov commented:
"""
Hi @pbrezina,
> Patch from sssd-2.x: [f1f9af5](https://github.com/SSSD/sssd/commit/f1f9af528f71f42ac41bb7a272f4f7d940fd3a0f)
>
> The interesting thing is that `watch->fde` should be freed with `watch` because it is its parent (see the code below) and there should not be any need to free it explicitly. I fixed it in sssd-2.x because valgrind kept complaining about it

Do you remember what was the case when valgrind complained? (i.e. how to reproduce)

Do you know if order of `talloc_free(watch_fd->fdevent)` and `talloc_free(watch_fd)` was important to fix complain?
I mean, would calling `talloc_free(watch_fd->fdevent)` from `watch_destructor()` or after `talloc_free(watch_fd)` make valgrding happy?

"""

See the full comment at https://github.com/SSSD/sssd/pull/930#issuecomment-550452609

[sssd PR#930][comment] [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

[pbrezina]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

pbrezina commented:
"""
> Do you remember what was the case when valgrind complained? (i.e. how to reproduce)

No... but I think it was visible during shutdown or something like that. Try to revert the patch (or whole commit history) and run it under valgrind.
> Do you know if order of `talloc_free(watch_fd->fdevent)` and `talloc_free(watch_fd)` was important to fix complain?
> I mean, would calling `talloc_free(watch_fd->fdevent)` from `watch_destructor()` or after `talloc_free(watch_fd)` make valgrding happy?

I do not remember if I tried to put it in the destructor.

"""

See the full comment at https://github.com/SSSD/sssd/pull/930#issuecomment-550984174

[sssd PR#930][comment] [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Title: #930: [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

alexey-tikhonov commented:
"""
I think I was wrong in mechanics how does this "use-after-free" bug happen. Actual patch will be totally different so I will close this PR.
"""

See the full comment at https://github.com/SSSD/sssd/pull/930#issuecomment-550992938

[sssd PR#930][closed] [WiP] SBUS: remove dbus fd from tevent loop when watch is removed

[alexey-tikhonov]

URL: https://github.com/SSSD/sssd/pull/930
Author: alexey-tikhonov
 Title: #930: [WiP] SBUS: remove dbus fd from tevent loop when watch is removed
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/930/head:pr930
git checkout pr930