Heap corruption

[bublina]

Hello,

 

I am passing some data from dll to labview. It is an array of clusters. Cluster looks like this:

typedef struct 
{
	LStrHandle longname;
	LStrHandle name;
	uint8_t intraonly;
	uint8_t lossless;
	uint8_t lossy;
} CodecInfo, **CodecInfoHdl;

typedef struct CodecInfoArr
{
	int32_t dimsize;
	CodecInfo Arr[1];
} CodecInfoArr, **CodecInfoArrHdl;

But I think it is reproducable with any kind of array of clusters where are other handles, like LStrHandle in my case.

The operation is pretty straightforward.

I check the incoming handle, I handle the situation where it is empty or filled with data and in the next step I have an empty handle to work with.

So far I have an array handle returned from DSNewHandle allocated for the resulting array size.

I start copying the data, but since there are strings involved, I call my general char2LStrHandle function to fill string handles.

void PrintChar2LVStrHandle(const char *charstr, LStrHandle *StrHandleP, bool forcenew = false)
{
	MgErr error = mgNoErr;
	std::string temp = "";
	if (charstr)
	{
		temp.assign(charstr);
	}
	if ((!IsHandleEmpty(StrHandleP)) && !forcenew)
	{
		error = DSDisposeHandle(*StrHandleP);
		if (error != mgNoErr)
			throw(WrpExcUser("PrintChar2LVStrHandle()", error));
	}
	*StrHandleP = (LStrHandle)(DSNewHandle(Offset(LStr, str) + sizeof(uChar)));
	if (*StrHandleP == nullptr)
		throw(WrpExcUser("Could not allocate string handle", CUSTOMERROR));
	//(**StrHandleP)->cnt = 0;
	error = LStrPrintf(*StrHandleP, (CStr)"%s", temp.c_str());
	if (error != mgNoErr)
		throw(WrpExcUser("Could not print to string handle", error));
}

There is a lot of junk&unnecesary code, but the core of the function is to check the handle, and if it is valid, just print into it. The function that checks the handle looks like this:

bool IsHandleEmpty(const LStrHandle *h)
{
	MgErr err, heaperr = mgNoErr;
	err = DSCheckHandle(*h);
	//heaperr = DSHeapCheck(0);

	if (err)
		return true;
	return false;
}

It is just a stupid wrapper around DSCheckHandle. 

 

Now the problem is, that with the way the array handle allocated the array elements, some handle pointers, from time to time point to memory so unfortunately, that the array elements that get processed later have string handle pointer pointing to memory that is already filled with data from previous array filling loop iterations. It is not the exactly same memory, since the data is little off (garbage, count is wrong etc.), but it is sufficient enough for the DSCheckHandle return, that the handle is valid. So the memory gets a treatment like it would be a real handle and it corrupts the labview heap, so when the data gets deallocated, LabView / runtime will crash.

 

So far I tackled the solution with simple optional parameter (bool forcenew = false) and went through all the function references in code to set the flag if the data comes from new array handles.

 

Finally the question: Is there more elegant solution to this, somehow correctly check the incoming string handle ?

[JKSH]

Hi,

 

the core of the function is to check the handle, and if it is valid, just print into it.

 

When would the LStrHandles ever be valid? If you allocate a new CodecInfo array, then all the LStrHandles are uninitialized (that means they are never valid).

 

So the solution is this: For all of the LStrHandles inside your new array of clusters, always call DSNewHandle() and never call DSCheckHandle().

 

 

the data is little off (garbage, count is wrong etc.), but it is sufficient enough for the DSCheckHandle return, that the handle is valid.

 

Like I mentioned before, your new LStrHandles are uninitialized pointers. You must never call a function (including DSCheckHandle()) to perform an action on an unitialized pointer. If you do, you will get undefined behaviour (and usually crash) -- This applies to all C/C++ code, not just LabVIEW interface code.

Edited July 19, 2015 by JKSH

[bublina]

Yes yes, you are completely right. I was kinda dull and supertired when finaly revealing where the problem of my crashing was, so I just lazy posted here

 

However, I think it is better to use DSNewHClr, that correctly initialized the handles to nullptr. It feels much safer, so all the if(*ptr) have predictible result.

 

Thx for answer.

[JKSH]

I think it is better to use DSNewHClr, that correctly initialized the handles to nullptr. It feels much safer, so all the if(*ptr) have predictible result.

 

Sounds good

 

Happy coding!

[Rolf Kalbermatter]

It's generally correct what JKSH writes when the handle containing the handles is allocated by yourself. However if that array of CodecInfo structs comes from LabVIEW there are strict rules that can be observed and must be followed when returning such data to LabVIEW.

Anything beyond the number of elements indicated in the array is uninitialized although the actual array handle space may be bigger but never smaller than needed for the number of valid elements.

The only exception to this are ampty array handles which can be both either a valid handle with a number of elements equal to 0 OR a null handle itself.

So when receiving handles from LabVIEW you should always check for null and depending on that do DSNewHandle/DSNewHClr or DSSetHandleSize. When resizing a handle existing elements need to be resized and appended elements always created. Removed elements when making an array smaller must always be recursively deallocated as they don't exist for LabVIEW anymore once the array length has been readjusted to a smaller size and would therefore create a memory leaks.

If you allocate both the array handle as well as the contained handles inside the array in the same code section without returning in between to the LabVIEW diagram it is entirely up to you if you want to use DSNewHandle or DSNewHClr, as the values inside the newly allocated array need to be initialized anyhow explicitly. The latter requires more execution time but may be faster if you need to initialize most elements in there to 0 or an empty handle anyways. Also it may be a little safer when someone later makes modifications to the code as null pointer dereferencing has a higher chance of crashing than accessing uninitialzed pointers, so debugging is easier.