On Fri, Aug 26, 2016 at 12:07:45PM +0200, Ross Finlayson wrote: > Yes, you are correct here. Fortunately, there is an easy fix (which I’ll add > to the code in the near future): Set “fNext” to NULL at the start of the > handler function (“sendNext()”). Ditto for the other places in the code > where the same thing is done.
That's exactly what my patch does. It just looks long, because there are so many places that lack these NULL assignments. Did you read it? > Are you actually seeing these ‘double frees’ in your own application? If you > are, then I’m a bit surprised, because nobody else has reported seeing these. > Yes, the problem that you note is real - but for most people, it apparently > doesn’t happen (because apparently - in most cases - “Medium” objects do not > get destroyed between the handling/scheduling of delayed tasks). But if > you’re actually seeing this happen, then it appears that your application is > doing something unusual, which makes me worried, because of what you wrote > next: Yes, I am. I guess I am combining two unusual things. Instead of using BasicTaskScheduler, I am using my own scheduler[1]. The other aspect is that I am testing the teardown path for a rtsp server. Only the combination runs into these double frees. In this scheduler, there is a particular assertion for catching some of these double frees, which can otherwise go unnoticed. If you add an assert(entry != NULL); to DelayQueue::removeEntry, you should be seeing the issue in the teardown path as well. Since an expired (invalid) entry is removed from the queue, looking up such a token should result in entry being NULL. > I hope you have read the FAQ (that everyone was asked to read before posting > to the mailing list :-) - specifically: > http://live555.com/liveMedia/faq.html#threads I did. See below. > You absolutely MUST NOT try to access a “TaskScheduler” (or any other LIVE555 > object) from more than one thread. As explained in the FAQ, the LIVE555 code > was designed to be run as a single-threaded event loop. (It *is* possible, > however, to run LIVE555 code from more than one thread, if each thread uses > its own “UsageEnvironment” and “TaskScheduler” object - again, as explained > in the FAQ.) Well, that FAQ entry is slightly wrong here. It really should say BasicTaskScheduler instead of TaskScheduler. If a downstream of liveMedia implements a subclass of TaskScheduler in a thread-safe way (and that includes never accessing other live555 objects from other threads), then accessing such a TaskScheduler subclass from another thread is indeed safe. In fact, even the FAQ advises that accessing TaskScheduler::triggerEvent is supposed to be thread-safe (even though BasicTaskScheduler::triggerEvent is not thread-safe due to the missing memory ordering and atomicity primitives). As you can see, the FAQ contradicts itself and only a look at the code reveals the truth. So given the above, implementing TaskScheduler-subclass that has a thread-safe triggerEvent method sounds like a good idea, doesn't it? There is one aspect where thread-safety is relevant to the double frees. By setting the watch variable from another thread (which is supposedly supported according to the FAQ), doEventLoop can be interrupted in unexpected states. Thus, the issue may indeed only be observable with another thread setting the watch variable. Helmut [1] Once it works practically, we can consider including it into liveMedia, but I caution that it really is Linux-only. _______________________________________________ live-devel mailing list live-devel@lists.live555.com http://lists.live555.com/mailman/listinfo/live-devel
