search

troglobit / mdnsd

Jeremie Miller's original mdnsd
BSD 3-Clause "New" or "Revised" License
55 stars 35 forks source link

Regression introduced in v0.11 #52

Closed MonkeyBreaker closed 1 year ago

MonkeyBreaker commented 2 years ago

Hi,

First of all, thank you for maintaining this great library. Second, I won't be able to describe precisely my issue, because I did not still fully grasped but I hope providing enough information to help fixing this issue. I, of course will try to help, but my understanding of the library is limited for the moment.

I deployed v0.11, and after some time (15min to some hours) mdnsd was crashing, I saw that a SIGSEGV was thrown.

Looking at the reason of this SIGSEGV, I got a double free. Continue my debugging, the back-trace showed the following:

  1. (src/mdnsd.c) main -> L.458
  2. (src/mdnsd.c) free_iface -> L.112
  3. (libmdnsd/mdnsd.c) mdnsd_free -> L.755
  4. (libmdnsd/mdnsd.c) _free_record -> L.399

As far as I understood the issue, it seems that function free_iface doesn't clean correctly the iface (I my be wrong). It is not easy to reproduce, but a brutal way I found out is to simply comment the entire if. It is a bit brutal, but the moment you start the application it will SIGSEGV, with the details I show before.

I was able for the moment to find a workaround, which for the moment seems to work (4 days running, no crash observed so far). In function free_iface, after the if condition, I call iface_free(iface). This function comes from addr.c and seems to clean "correctly" the interface.

I am not sure this is the correct fix, but for the moment It seems to work.

BTW, I put regression in the name of the issue, because I tested commenting the same if with the commit before 872d508, and no SIGSEGV observed.

MonkeyBreaker commented 2 years ago

Okay, looking at the difference between the refactor in https://github.com/troglobit/mdnsd/commit/872d508d1a1f4c72618876d6780bd9ff4157373d, it seems that the free_iface does too much or not enough depending when it is called. Below I list the 3 parts where free_iface is called and after I put a link to how it was previous the change:

In the first case, what is missing is a call to FD_CLR(iface->sd, &fds); and also a call to iface_free(iface);. In the second case, now mdnsd_shutdown(iface->mdns); and mdnsd_free(iface->mdns); are called, were previously they weren't. Last case it is exactly the same.

Let's hope this additional information helps !

troglobit commented 2 years ago

Woah, this issue had completely slipped below my radar. I'll have a look at the PR this weekend!