View Issue Details

IDProjectCategoryView StatusLast Update
0002683Ham Radio DeluxeBugpublic2018-05-13 15:26
ReporterK7ZCZ 
Assigned ToK7ZCZ 
PriorityhighSeveritycrashReproducibilityrandom
Status closedResolutionfixed 
PlatformIntel i7-5960XOSWindows 10 Professional x64OS Version16299
Product Version6.4.0.805 
Target VersionFixed in Version6.4.0.840 
Summary0002683: Logbook: potential for fatal deadlock
DescriptionThe 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 ReproduceThere 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.
TagsNo tags attached.
ModuleLogbook
Sub-ModuleFunctional
Testing N/A

Relationships

Activities

K7ZCZ

2018-04-20 21:59

manager   ~0004864

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.

K7ZCZ

2018-04-20 22:22

manager   ~0004867

fixed with this checkin
https://hrdsoftware.visualstudio.com/HRD/_versionControl/changeset/4061

WA9PIE

2018-05-12 01:09

administrator   ~0005013

Closed and waiting on May 2018 release

Issue History

Date Modified Username Field Change
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 => 6.4.0.837
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 6.4.0.837 => 6.4.0.840
2018-05-13 15:26 WA9PIE Project 3 - Current Dev List => Ham Radio Deluxe