case study: data type bug in the code... watch out!

[ElectroLund]

I ran into a code bug that took a while to figure out and I thought I'd pass along the story to warn others.

The following pseudo code contains the bug.  The symptom was that mid-execution, the value of position was getting corrupted and thus causing a GPF at the sprintf.

int CVICALLBACK ServoAutoCalibrate (int panel, int control, int event, void *callbackData, int eventData1, int eventData2)
{
	float pwm;
	int position;
	int timerInterval, timerState;
	
	switch (event)
	{
		case EVENT_COMMIT:

			GetAsyncTimerAttribute(ljAsyncTimer, ASYNC_ATTR_INTERVAL, &timerInterval);
			
			for (int k=0; k < CAL_AVERAGE_SIZE * timerInterval; k++)
			{					
				GetAsyncTimerAttribute(ljAsyncTimer, ASYNC_ATTR_ENABLED, &timerState);
				
				if (timerState == OFF)
					break;
				
				// delay a bit more than the timer period
				DelayWithEventProcessing(timerInterval * 1.01);
			}
		
			sprintf(statusStr, 
					  "Old value = %1.3f pwm",
						systemConfig.servoDuty[position]);
			
			MessagePopup(statusPanel, statusStr, MESSAGE_NORMAL);
			break;
	}
	return 0;
}

See the bug?

I had mistakenly declared timerInterval as an int, rather than a double.  And because I'm not in the habit of error-checking the CVI standard library, the call to GetAsyncTimerAttribute blindly overfilled timerInterval with its double value, which then overwrote the value of position.

position was the collateral damage because it's declared before timerInterval.

[AllenHuffman]

Great advice!

I was just thinking about these types of potential issues (passback variables) the other day. At a previous job, I had the task of making an entire project more bulletproof by making sure we never used strcpy(), strcat(), sprintf(), etc. and instead used the safer versions like strncpy(), strncat(), snprintf(), etc. During this, I also updated some of our internal calls where a return variable was passed in by address -- basically, anything passed in by address also got a sizeof() value, which would then be used to detect issues such as this (quickly typed pseudo code):

bool GetCoordinates (int *x, int *y)
{
    // Bad things can happen if they don't pass in an int...
    return true;
}

...changed to the safer, but bulkier...

bool GetCoordinates (int *x, size_t ysize, int *y, size_t ysize)
{
    bool status;
    if ( (xsize != sizeof(*x) ) || (ysize != sizeof(*y) )
    {
        status = false;
    }
    else
    {
        // Do stuff...
        status = true;
    }
    return status;
}

It added to the code size and execution time, but our product couldn't be allowed to crash form a pointer issue or memory corruption, so that is the choice we made.

It's a tough habit to get in to, but once you start coding with the thought of "if I pass in the address of something, also pass in the size and verify" it eliminates these issues.

Unfortunately, there's not much one can do with pre-existing library code that doesn't do this 😞

[gdargaud]

I see another potential problem with your code. I don't see your declaration of statusStr, so I assume it's a global. But callbacks for async timers run in a separate thread, so if you use that variable in your main thread, you need to protect it (semaphore, etc).

[ElectroLund]

That's a very good point, gdargaud.  My statusStr is not thread safe.