URL: https://github.com/SSSD/sssd/pull/724 Author: alexey-tikhonov Title: #724: COMPONENT: util/tev_curl Action: opened
PR body: """ Fixes double free error in schedule_fd_processing() (prevents deletion of already executed timer)
Resolves: https://pagure.io/SSSD/sssd/issue/3917 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/724/head:pr724 git checkout pr724
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
jhrozek commented: """ Hi Alexey, thank you very much for the patch and especially for diving into the code.
I admit that I don't remember the details about the tcurl module anymore. Could you please explain the double-free in more detail? Looking at tevent docs, they say that the tevent timer is freed automatically and looking at the code, the `schedule_fd_processing` function is only ever called from `tcurl_init` which should be a one-time operation. So currently I'm not sure how could the function delete a timer that was already executed? """
See the full comment at https://github.com/SSSD/sssd/pull/724#issuecomment-451290588
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
alexey-tikhonov commented: """
Could you please explain the double-free in more detail? Looking at tevent docs, they say that the tevent timer is freed automatically
That's correct.
and looking at the code, the `schedule_fd_processing` function is only ever called from `tcurl_init` which should be a one-time operation. So currently I'm not sure how could the function delete a timer that was already executed?
`schedule_fd_processing` is not "called" from `tcurl_init`. It is setup as callback there. So `libcurl` calls `schedule_fd_processing` every time it wants to have (new) timer setup. And (in kcm intg test) it happens quite a lot of times.
Being called `schedule_fd_processing` wants to delete previous timer before creating new one. That's basically proper action. The problem is it sometimes tries to delete non-existent (already executed and deleted by libtevent) timer. This is "double free" case.
Proposed patch sets timer pointer in `tcurl_ctx` to NULL at the end of timer handler (actually can be placed anywhere in the handler: handler called => timer deleted by libtevent). That prevents `schedule_fd_processing` from freeing already freed memory. And I think it is good idea overall to not have pointers to freed memory. This should be safe operation since all processes are single-thread processes (as far as I understand). """
See the full comment at https://github.com/SSSD/sssd/pull/724#issuecomment-451390544
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
alexey-tikhonov commented: """
Could you please explain the double-free in more detail? Looking at tevent docs, they say that the tevent timer is freed automatically
That's correct.
and looking at the code, the `schedule_fd_processing` function is only ever called from `tcurl_init` which should be a one-time operation. So currently I'm not sure how could the function delete a timer that was already executed?
`schedule_fd_processing` is not "called" from `tcurl_init`. It is setup as callback there. So `libcurl` calls `schedule_fd_processing` every time it wants to have (new) timer setup. And (in kcm intg test) it happens quite a lot of times.
Being called `schedule_fd_processing` wants to delete previous timer before creating new one. That's basically proper action. The problem is it sometimes tries to delete non-existent (already executed and deleted by libtevent) timer. This is "double free" case.
Proposed patch sets timer pointer in `tcurl_ctx` to NULL at the end of timer handler (actually can be placed anywhere in the handler: handler called => timer deleted by libtevent). That prevents `schedule_fd_processing` from freeing already freed memory. And I think it is good idea overall to not have pointers to freed memory (dangling pointers). This should be safe operation since all processes are single-thread processes (as far as I understand). """
See the full comment at https://github.com/SSSD/sssd/pull/724#issuecomment-451390544
URL: https://github.com/SSSD/sssd/pull/724 Author: alexey-tikhonov Title: #724: COMPONENT: util/tev_curl Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/724/head:pr724 git checkout pr724
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
jhrozek commented: """ I used your nice explanation to improve the commit message and pushed the fix to master: * 15bde7dab466fc4f2719ce709de9dac7e1e10de8 """
See the full comment at https://github.com/SSSD/sssd/pull/724#issuecomment-451766724
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/724 Title: #724: COMPONENT: util/tev_curl
jhrozek commented: """ also pushed to sssd-1-16: 37718f82ce55b3a873e4157adee1bfdaab6f14dc """
See the full comment at https://github.com/SSSD/sssd/pull/724#issuecomment-505576846
sssd-devel@lists.fedorahosted.org