Thread sanitizer reveals missing locks

[jefftrull]

I recently ran a client's software with gcc's thread sanitizer enabled. It revealed some issues in mosquitto that I've worked around in a fork but I felt I should document them here. The problems are:

• Inconsistent protection of mosq->in_callback and mosq->state by their respective mutexes
• mosq->sock may need its own mutex

I've attached a very rough patch showing where the issues are.

[ralight]

Thanks for this. Out of interest, is the code using loop_start(), or threads external to the library?

[jefftrull]

External threads in this case. Do you suspect a user error :)

(we are calling mosquitto_threaded_set)

[ralight]

Not at all, it's just good to know the situation.

I've not look at it properly yet, but I'm curious to know your reasoning as to why you've removed the mutex from the callback calls.

[jefftrull]

I found that mosquitto_reinitialise was getting called from them and thus the mutexes were reset, causing the following unlock to silently fail... Could this be something we're doing wrong?

[jefftrull]

mosq->in_callback still gets protected

[jefftrull]

I'm sorry, mosquitto_reinitialise is not getting called again. Something else is happening in the callback that is messing up the locks somehow. Investigating...

[jefftrull]

OK, thanks for pointing this out. The changes in locking of callback_mutex were a rabbit hole triggered by the fact that we call mosquitto_disconnect from our main thread, taking a path where callback_mutex was not already held. It looks like the (much) easier fix is to change a single test in packet_mosq.c - the value of in_callback is only interesting in non-threaded mode there, so it's safe.
New diff attached

[jefftrull]

Further investigation reveals that the first change in that last diff (removing the access to mosq->sock) is also unnecessary. Every remaining change protects mosq->state.

[ralight]

Thanks, I've now added a commit that implements this.