Evil Linux bug #100

New Issue

muellerr · 2020-06-06T16:15:17+02:00

muellerr commented

2020-06-06 16:15:17 +02:00

This includes general improvements and a very important bugfix.

The bug is a perfect example for how unsafe C can be. Basically one letter was missing, and it was enough to cause
an insidious bug which was hard to track down. I'll write down a bit because it is very interesting to see how missleading
issues like these can be.

Culprit code:

PosixThread::PosixThread(const char* name_, int priority_, size_t stackSize_):
		thread(0),priority(priority_),stackSize(stackSize_) {
	strcpy(name,name_);
}

POSIX thread names are restricted to 16 chars.
The name buffer for some reason only had 10 entries.
But the biggest problem was that a strcpy was performed instead of a strncpy.
strcpy is extremely unsafe, it simply copies everything till '\0'.
So, me as a programmer who has no idea of that maximum name length, created a new fixed timeslot task
with the beautiful long name POLLING_SEQUENCE_TABLE_DEFAULT.

/* Polling Sequence Table Default */
FixedTimeslotTaskIF * PollingSequenceTableTaskDefault =
			TaskFactory::instance()-> createFixedTimeslotTask(
					"POLLING_SEQUENCE_TABLE_DEFAULT", 4,
					PeriodicTaskIF::MINIMUM_STACK_SIZE, 0.4, nullptr);
result = pst::pollingSequenceInitDefault(PollingSequenceTableTaskDefault);
if (result != HasReturnvaluesIF::RETURN_OK) {
	sif::error << "creating PST failed" << std::endl;
}

So, first issue I encountered was that the msg_max value of the system (default 10) needed for the posix message queueswas too low for DHB (20) and event manager(80).
I fixed that. Next issue: createTask complains. It can not allocate enough memory. So eventually I print out or trace that value: It's some astronomical number (4 * 10^11 MB or something).

So first guess is that that value is uninitialized. It is not.
In fact, when I printed out the value at some other spot in the code, its the MINIMUM_STACK_SIZE for Linux.
So I look everywhere, wondering why the task size is changed. Best thing is, when I make the cached stackSize member protected by moving it up, the size was not changed. Endless confusion.
Eventually after 40 min, I simply set the stack size to the
MINIMUM_STACK_SIZE inside the createTask function.
Great, now its not complaining anymore. Next error: name invalid.

Oh, okay, only 16 chars allowed, alright, I will just truncate the name. How is it stored anyway.. A char[10]?? Odd..
how is it set?

And then I saw the strcpy. the strcpy, being a dumb and dangeorous function, copied
the passed 20-30 char name into the member name and conveniently overwrote one or some members after name, including the stackSize (I guess those are contiguous in memory).
The name was checked after the stack size is set, so i was looking in the wrong place all the time.

This includes general improvements and a very important bugfix. The bug is a perfect example for how unsafe C can be. Basically one letter was missing, and it was enough to cause an insidious bug which was hard to track down. I'll write down a bit because it is very interesting to see how missleading issues like these can be. Culprit code: ```cpp PosixThread::PosixThread(const char* name_, int priority_, size_t stackSize_): thread(0),priority(priority_),stackSize(stackSize_) { strcpy(name,name_); } ``` POSIX thread names are restricted to 16 chars. The name buffer for some reason only had 10 entries. But the biggest problem was that a strcpy was performed instead of a strncpy. strcpy is extremely unsafe, it simply copies everything till '\0'. So, me as a programmer who has no idea of that maximum name length, created a new fixed timeslot task with the beautiful long name POLLING_SEQUENCE_TABLE_DEFAULT. ```cpp /* Polling Sequence Table Default */ FixedTimeslotTaskIF * PollingSequenceTableTaskDefault = TaskFactory::instance()-> createFixedTimeslotTask( "POLLING_SEQUENCE_TABLE_DEFAULT", 4, PeriodicTaskIF::MINIMUM_STACK_SIZE, 0.4, nullptr); result = pst::pollingSequenceInitDefault(PollingSequenceTableTaskDefault); if (result != HasReturnvaluesIF::RETURN_OK) { sif::error << "creating PST failed" << std::endl; } ``` So, first issue I encountered was that the msg_max value of the system (default 10) needed for the posix message queueswas too low for DHB (20) and event manager(80). I fixed that. Next issue: createTask complains. It can not allocate enough memory. So eventually I print out or trace that value: It's some astronomical number (4 * 10^11 MB or something). So first guess is that that value is uninitialized. It is not. In fact, when I printed out the value at some other spot in the code, its the MINIMUM_STACK_SIZE for Linux. So I look everywhere, wondering why the task size is changed. Best thing is, when I make the cached stackSize member protected by moving it up, the size was not changed. Endless confusion. Eventually after 40 min, I simply set the stack size to the MINIMUM_STACK_SIZE inside the createTask function. Great, now its not complaining anymore. Next error: name invalid. Oh, okay, only 16 chars allowed, alright, I will just truncate the name. How is it stored anyway.. A char[10]?? Odd.. how is it set? And then I saw the strcpy. the strcpy, being a dumb and dangeorous function, copied the passed 20-30 char name into the member name and conveniently overwrote one or some members after name, including the stackSize (I guess those are contiguous in memory). The name was checked after the stack size is set, so i was looking in the wrong place all the time.

muellerr added the

bug

label 2020-06-06 16:15:17 +02:00

muellerr added the

feature

label 2020-06-06 16:16:26 +02:00

muellerr referenced a pull request that will close this issue

2020-06-06 16:17:07 +02:00

linux important bugfix and general improvements #101

muellerr changed title from ~~Very annoying Linux bug~~ to Evil Linux bug

2020-06-08 11:33:19 +02:00

gaisser self-assigned this 2020-06-08 11:54:11 +02:00