View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0002683||Ham Radio Deluxe||Bug||public||2018-04-17 23:05||2018-05-13 15:26|
|Platform||Intel i7-5960X||OS||Windows 10 Professional x64||OS Version||16299|
|Target Version||Fixed in Version||184.108.40.2060|
|Summary||0002683: Logbook: potential for fatal deadlock|
|Description||The Logook features a window tab called "Logfile". This tab simply lists text messages that are displayed by various parts of the logbook's code as a journal to show what might be going on in the logbook's internal code. This is useful for support, showing warning messages, and so on.|
There is a fundamental flaw in this implementation which can cause a deadlock against the main thread of the paplication resulting in a crash. There's no real defense against the deadlock except for luck. The code should be fixed to be made reliable, since we have no information on how often these deadlocks occurr, or how they might happen in scenarios exercised by the released application. (Note that the the minidump code collects crashes, not deadlocks or not livelocks.)
The logfile window (and the tab and frame that it is inside) is owned by the main thread of the application. The CWinApp-derived class in the application's implementation features an accessor which writes to the logfile window. This method is AddLogfile(), which takes a format string and variadic parameters. The resulting string is added to the window.
The application class also features a critical section (m_csectionConsole, wrapped by GetConsoleLock()).
The applicatoin further features several background threads. Anythread can (and does) access the AddLogfile() method on the application to generate log messages.
The implementation of AddLogfile() calls methods on the list control owned by the main window, which calls SendMessage() to the window. SendMessage() forces a context switch to the window's owner, which is the main thread. The call blocks until the main thread is able to service the call.
Let's say the main thread is performing an operation; so is a background thread. That's certainly by design: that's why we have multiple threads.
If any secondary thread calls AddLogfile(), it will acquire the critical section and send a message to the main thread. The main thread continues processing, which might include itself posting a message to the logfile with a call to AddLogFile(). That call will try to acquire the same console lock critical section. Since the main thread blocks on the critical thread owned by the worker thread, the worker thread will never get an answer to its SendMessage() call and will never release the critical section that the main thread is trying to acquire.
This deadlock can happen at any point in the application; there's no correctness protection for it, and is likely the cause of other bugs.
The logfile feature is certainly worth keeping. It should be rewritten to be lock-free and asynchornous. This wil be a more efficient implementation with no forced context swithcing; but far more importantly it will have zeeor change of deadlocking the application.
|Steps To Reproduce||There are no specific steps to reproduce this issue. It is a code quality issue that can be deonstrated by static inspectoin of the code as outlined above.|
|Tags||No tags attached.|
This code has a terrible implementation; I've rewritten it to decouple any client thread from the main thread that owns the window, and that makes deadlocks impossible. That rewrite has the side-effect of eliminating memory leaks and crashes at shutdown.
There's no real way to tell how much, how often this issue actually affected customers, but any improvement is better. And, in this case, the code was bad enough that it blocked progress on other work.
fixed with this checkin
||Closed and waiting on May 2018 release|
|2018-04-17 23:05||K7ZCZ||New Issue|
|2018-04-20 21:59||K7ZCZ||Note Added: 0004864|
|2018-04-20 22:22||K7ZCZ||Assigned To||=> K7ZCZ|
|2018-04-20 22:22||K7ZCZ||Status||new => resolved|
|2018-04-20 22:22||K7ZCZ||Resolution||open => fixed|
|2018-04-20 22:22||K7ZCZ||Note Added: 0004867|
|2018-05-03 20:10||K7ZCZ||Fixed in Version||=> 220.127.116.117|
|2018-05-12 00:37||WA9PIE||Steps to Reproduce Updated||View Revisions|
|2018-05-12 00:37||WA9PIE||Testing||Not Started => N/A|
|2018-05-12 01:09||WA9PIE||Note Added: 0005013|
|2018-05-12 01:09||WA9PIE||Status||resolved => closed|
|2018-05-13 15:25||WA9PIE||Fixed in Version||18.104.22.1687 => 22.214.171.1240|
|2018-05-13 15:26||WA9PIE||Project||3 - Current Dev List => Ham Radio Deluxe|