search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.15k stars 2.47k forks source link

crash on exit in logger shutdown #1904

Closed kazzmir closed 4 years ago

kazzmir commented 4 years ago

Janus at master aa4168e7afc7f358eee4b36b0108dcd79e565c3b segfaults on exit due to the log.c lock being cleared and then accessed.

janus_log_init calls g_mutex_init(&lock) and also creates the printthread, janus_log_thread janus_termination_handler is registered as an atexit handler in janus.c When exit() is called, janus_termination_handler calls janus_log_destroy, which will stop the printthread (and wait for it). The last few lines of janus_log_thread will clear the lock mutex.

https://github.com/meetecho/janus-gateway/blob/aa4168e7afc7f358eee4b36b0108dcd79e565c3b/log.c#L164

janus_termination_handler continues to call janus_log_set_loggers(NULL), which tries to lock the mutex. Since the lock has already been cleared this segfaults.

https://github.com/meetecho/janus-gateway/blob/aa4168e7afc7f358eee4b36b0108dcd79e565c3b/log.c#L250

I guess calling g_mutex_clear(lock) is being a good citizen and cleaning up resources, but it could possibly be elided entirely. Otherwise maybe there should be a janus_log_cleanup() method that janus_termination_handler can call that will clear the lock and the cond variables.

lminiero commented 4 years ago

Thanks for the heads up, I'll have a look at this tomorrow when I'm back at the office.

lminiero commented 4 years ago

Not segfaulting to me, though... possibly a race condition?

lminiero commented 4 years ago

Since the issue was janus_log_set_loggers being called after the thread invalidated the mutex, I simply moved it in the part where the thread does its cleanup, instead of having it in janus.c. I noticed we weren't pushing the optionally dangling pieces of logs to loggers as well, so I fixed that too.

Please let me know if you're still experiencing the issue (as I said I can't replicate it myself).

kazzmir commented 4 years ago

I can get it to crash every time, so not sure what is different about my setup. But anyway with the fix you put in janus no longer crashes.

Before b129d78:

[Tue Jan  7 12:38:32 2020] JANUS Streaming plugin destroyed!
[Tue Jan  7 12:38:32 2020] Leaving VideoRoom handler thread
[Tue Jan  7 12:38:32 2020] Leaving RTCP thread for RTP forwarders...
[Tue Jan  7 12:38:32 2020] JANUS VideoRoom plugin destroyed!
[Tue Jan  7 12:38:32 2020] Leaving NoSIP handler thread
[Tue Jan  7 12:38:32 2020] JANUS NoSIP plugin destroyed!
[Tue Jan  7 12:38:32 2020] Closing event handlers:
Bye!
GLib (gthread-posix.c): Unexpected error from C library during 'pthread_mutex_lock': Invalid argument.  Aborting.

After:

[Tue Jan  7 12:40:27 2020] JANUS Streaming plugin destroyed!
[Tue Jan  7 12:40:27 2020] Leaving VideoRoom handler thread
[Tue Jan  7 12:40:27 2020] Leaving RTCP thread for RTP forwarders...
[Tue Jan  7 12:40:27 2020] JANUS VideoRoom plugin destroyed!
[Tue Jan  7 12:40:27 2020] Leaving NoSIP handler thread
[Tue Jan  7 12:40:27 2020] JANUS NoSIP plugin destroyed!
[Tue Jan  7 12:40:27 2020] Closing event handlers:
Bye!
kazzmir commented 4 years ago

I am eager to use a released version of janus with this fix. Will v0.8.1 come soon?

BTW I realize this is not the right place for such a question, what is a better venue?

lminiero commented 4 years ago

Discussions belong on the group. Not sure when we'll tag 0.8.1, I'll probably wait for a couple more fixes to land (there are a few issues and PRs pending that I have to look at).