Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signal handler is not safe #41

Closed
NomAnor opened this issue Jan 21, 2018 · 7 comments
Closed

Signal handler is not safe #41

NomAnor opened this issue Jan 21, 2018 · 7 comments

Comments

@NomAnor
Copy link

NomAnor commented Jan 21, 2018

Signal handlers work like interrupts which means they need to be reentrant. According to the posix man page (http://man7.org/linux/man-pages/man7/signal-safety.7.html) only a few functions are safe to be used in a signal handler.

Looking at the C++ Standard (http://en.cppreference.com/w/cpp/utility/program/signal) neither delete nor exit() are safe.

I'm not sure what's the best way to handle the signals but it seems the only safe thing is to set a flag (std::atomic, std::sig_atomic_t, or a C equivalent) and then poll that flag in the main loop.

@itay-grudev
Copy link
Owner

Yhea. The more time I spend thinking about this library, the more potential problems I find. But there just are very few things one can do to bypass the limitations of OS systems, race-conditions, memory access and etc.

There might be a solution though. Since I came up with a hack that deals with crashed QSharedMemory blocks, I might skip the deallocation at all. Thinking about it, even if the program gets into "undefined behaviour" the solution is still good. If it actually calls the shared memory destructors - even better.

@itay-grudev
Copy link
Owner

I'll take this under consideration for the next version when I have time. I'll leave your issue open.

@NomAnor
Copy link
Author

NomAnor commented Jan 21, 2018

Looking at the Qt source and the Linux man pages it seems it's impossible to guarantee shared memory deletion when a program crashes.

One Problem for the single instance functionality is that we don't know whether all processes which use the shared memory are started because the user can always start another one. So marking the shared memory for deletion right after we created it (so the OS removes it when the process is removed. The memory map stays valid) is not possible because processes started after that can't open the shared memory (the name is already removed).

When the Posix shm functions are used you can't read the current number of mapped processes so you can't unlink it when that count reaches 0. Qt leaks the memory always.

// On non-QNX systems (tested Linux and Haiku), the st_nlink field is always 1,
// so we'll simply leak the shared memory files.
cleanHandle();

When the SystemV shm functions are used (as on my System) on some implementations it is possible to mark the memory for deletion and open it afterwards. But there is no gurantee that works so its useless.

According to the Qt documentation it works as expected on Windows.

I think it is easier to remove the crash handling. It complicates the code but gives no more guarantees (undefined behaviour and so) that the memory is deleted. Document the problematic behaviour and implement a way to force a process to become the primary instance so user could force it with a command line argument.
What happens if the last process crashes (the memory is leaked) and then I start a new process? Maybe become the primary if the QLocalSocket can't connect?

https://linux.die.net/man/2/shmctl
https://linux.die.net/man/3/shm_open
https://github.com/qt/qtbase/blob/5.10/src/corelib/kernel/qsharedmemory_posix.cpp
https://github.com/qt/qtbase/blob/5.10/src/corelib/kernel/qsharedmemory_systemv.cpp
https://stackoverflow.com/questions/13377982/remove-posix-shared-memory-when-not-in-use

@itay-grudev
Copy link
Owner

That is true, but the hack is quite simple. You initialise the shared memory block. Than free it. At this point the OS does it's cleanup. And when you initialise it again even if the original process crashed there is no dead shared memory instance.

See: singleapplication.cpp:377

@NomAnor
Copy link
Author

NomAnor commented Jan 21, 2018

I see, that will remove any leaked memory if I start a new instance and no other instance is running.

With removing the crash handling I meant the signal handler.

@itay-grudev
Copy link
Owner

Yhea. I was planning to make a major overhaul of the library. I just haven't had the time. One of the points was to get rid of the signal handlers. The other theoretical thingy was to add support for promoting a secondary instance to a primary whenever the primary is closed.

@itay-grudev
Copy link
Owner

The library no longer uses signal handling for deallocating the shared memory block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants