[Bug] Bug in use of STL queue with asynchronous signals [sf#7]

[crayzeewulf]

Reported by crayzeewulf on 2008-10-03 23:12 UTC
See the following message for a full description:

https://sourceforge.net/forum/message.php?msg\_id=5363412

By: raywilhe

In tracking down program crashes, I think I might have found a bug in the libserial
read code.

From what I can tell, serial port reads happen through a signal handler function
which fills a stl queue with data. The SerialPort::Read() functions then pull
data from that stl queue.

The stl queue that passes information between the SignalHandler and the
SerialPort::Read() functions has no protections (e.g. mutex) to prevent simultaneous
access to the stl queue.

Since the SIGIO signal can interrupt the user's thread at anytime, this can
cause problems if you are lucky enough to have the folllowing happen:

1. User Calls SerialPort::ReadByte()
2. In the middle of executing the mInputBuffer.pop() function call
   the SIGIO signal is raised and the SerialPort::HandlePosixSignal()
   gets immediately called
3. In the SerialPort::HandlePosixSignal() function, the mInputBuffer.push()
   function is called
4. Execution returns back to SerialPort::ReadByte(), and the mInputBuffer.pop()
   finishes
5. Now the stl queue is corrupted and the random crashes start.....

Please let me know if anybody can confirm/deny that this is a problem. This
is my first time looking at the internals of libSerial, but thus far I am fairly
sure the above described problem is the smoking gun in my case.

[crayzeewulf]

Commented by raywilhe on 2008-10-07 20:32 UTC
I just wanted to post my solution. All it does is replace the std::queue<> with a ThreadSafeQueue<>. The ThreadSafeQueue just wraps a mutex around all queue operations. Obviously, since I am posting this in a forum, any and all are free to use this code (tidbit):

template<typename TYPE>
class ThreadSafeQueue : protected std::queue<TYPE>
{
public:
ThreadSafeQueue()
{
pthread_mutex_init(&_m, NULL);
}
~ThreadSafeQueue()
{
pthread_mutex_destroy(&_m);
}

void push(const TYPE& val)
{
pthread_mutex_lock(&_m);
std::queue<TYPE>::push(val);
pthread_mutex_unlock(&_m);
}
void pop()
{
pthread_mutex_lock(&_m);
std::queue<TYPE>::pop();
pthread_mutex_unlock(&_m);
}
bool empty() const
{
pthread_mutex_lock(&_m);
bool retVal = std::queue<TYPE>::empty();
pthread_mutex_unlock(&_m);
return (retVal);
}

unsigned int size() const
{
pthread_mutex_lock(const_cast<pthread_mutex_t*>(&_m));
unsigned int retVal = std::queue<TYPE>::size();
pthread_mutex_unlock(const_cast<pthread_mutex_t*>(&_m));
return (retVal);
}

const TYPE& front() const
{
pthread_mutex_lock(const_cast<pthread_mutex_t*>(&_m));
const TYPE& retVal = std::queue<TYPE>::front();
pthread_mutex_unlock(const_cast<pthread_mutex_t*>(&_m));
return (retVal);
}

const TYPE& back() const
{
pthread_mutex_lock(const_cast<pthread_mutex_t*>(&_m));
const TYPE& retVal = std::queue<TYPE>::back();
pthread_mutex_unlock(const_cast<pthread_mutex_t*>(&_m));
return (retVal);
}

TYPE& front()
{
pthread_mutex_lock(&_m);
TYPE& retVal = std::queue<TYPE>::front();
pthread_mutex_unlock(&_m);
return (retVal);
}

TYPE& back()
{
pthread_mutex_lock(&_m);
TYPE& retVal = std::queue<TYPE>::back();
pthread_mutex_unlock(&_m);
return (retVal);
}

protected:
pthread_mutex_t _m;

};

[crayzeewulf]

Commented by crayzeewulf on 2008-10-11 21:44 UTC
raywilhe:

Thank you very much for posting your solution.

Crayzee Wulf

[crayzeewulf]

Commented by tommysp on 2011-01-07 14:56 UTC
@raywilhe: thank you too, for your posting, it brought me on the right way to fix some "random crashes".

But as I discorverd, using mutex_lock inside an Signal handler is not such a good thing. It causes some awfull deadlocks. (not often, but it does)

This is also mentioned in the man page.

"ASYNC-SIGNAL SAFETY
The mutex functions are not async-signal safe. What this means is that
they should not be called from a signal handler. In particular, calling
pthread_mutex_lock or pthread_mutex_unlock from a signal handler
may deadlock the calling thread."

I suggest the use of a extra thread to handle the work of the SIGINT, and notify it in the SignalHandler only. In the thread it is ovious to use the locks.
Any better idea?

[crayzeewulf]

Commented by crayzeewulf on 2011-01-07 21:50 UTC
Tommy:

Thanks for the report. I think your analysis makes sense. I am going to try and reproduce this at my end. Placing a mutex around the access to the STL queue.

Thanks,

CrayzeeWulf

[crayzeewulf]

Commented by oschwa2s on 2013-11-25 19:41 UTC
@crayzeewulf: Did you test that so far? The project is stuck on a five year old release candidate with known bugs, which is a pity for such a useful lib. As I am currently working with libSerial, I will test it anyways and propose a patch if it's stable.