Incorrect Memory Allocation

[j-medland]

I opened a PR #372 to address a memory allocation error in the function below

grpc-labview/src/grpc_interop.cc

Lines 26 to 38 in 4026aa8

	void OccurServerEvent(LVUserEventRef event, gRPCid* data, std::string eventMethodName)
	{
	LStr* lvMethodName = (LStr*)malloc(sizeof(int32_t) + eventMethodName.length() + 1);
	lvMethodName->cnt = eventMethodName.length();
	memcpy(lvMethodName->str, eventMethodName.c_str(), eventMethodName.length());
	GeneralMethodEventData eventData;
	eventData.methodData = data;
	eventData.methodName = &lvMethodName;
	auto error = PostUserEvent(event, &eventData);
	free(lvMethodName);
	}

I thought I would expand here what the issue is and if you would like to specify a particular approach to fixing it then I would be able to submit a new PR.

The issue is whilst copying a std::string into a LabVIEW String you are not allocating the correct number of bytes for a LabVIEW String on either 32 or 64 bit systems.

On a 32-bit system the first 4 bytes of an LStr in memory will contain the string length and the remaining bytes will store the characters of the string - there is no need for a null termination byte to be accounted for so allocating sizeof(int32_t) + string.length() + 1 makes a buffer which is one byte too large which I appreciate is not a huge problem but still unnecessary. You can also just use string.data() instead of string.c_str() for the copy.

On a 64-bit system the first 4 bytes of an LStr in memory will be the size of the string, then there will be 4 padding bytes for memory alignment, then the remaining bytes will store the characters of the string. So your memory allocation is now 3 bytes short!

There are multiple options to fix this.

If you want to use a malloc and free then you could use sizeof(LStr) + string.length() - 1 to determine the number of bytes to allocate.

This works because the definition of LStr as

    struct LStr {
        int32_t cnt; /* number of bytes that follow */
        char str[1]; /* cnt bytes */
    };

... means that any padding between cnt and the first string character byte is accounted for, so you just have to add the number of bytes in the string minus one byte to get the total require memory size.

An alternative is to use NumericArrayResize to allocate the memory, as this handles different system bitness and you already have a function which will allocate and copy a std::string which I mention in my PR #372 .

The only part missing for this approach is to provide a DSDisposeHandle for Linux based systems.

If you think I have made a mistake in my analysis, please let me know.

Cheers

John

AB#2850320

[RolfKal]

Actually, I suspect something else here. A char is 1 byte and as such has only a 1 byte alignment requirement. I have programmed LabVIEW strings in C extensively both in 32-bit and 64-bit and never have come across your alignment issue mentioned here, and if I had I would have been stumped as it doesn't match the alignment rules for any C ABI I know of.

Your claim would be correct for an array of doubles but for slightly different reasons than you explain. A double has always a prefered alignment of 8 bytes, and most C compilers use a default alignment of 8 bytes too, so it would always end up with those 32-bit filler bytes between the size and the first element of the array. BUT! LabVIEW for Windows 32-bit uses packed memory alignment for all its data structures for historical reasons. Those bytes counted when a typical computer had only 4 MB of memory and most users felt that 8MB of RAM was an excessive requirement, back in 1992 when LabVIEW for Windows was released.

Most C compilers will however calculate sizeof(LStr) as 8 (if the default alignment of 8 is active). This is not because of filler bytes after the cnt element but because of filler bytes after the str element. The C compiler wants to align the begin of the structure to a 4 byte boundary because of the first int32 element in the structure. And it will expand the structure at the end such that an LStr str[10] for instance will assure that each element starts at a multiple of 4. Some versions of Visual C can be a bit inconsistent here. It may correctly use the pack(1) attribute (in the case of Windows 32-bit as that is what LabVIEW declares all those structs to be) but if you initialize the structure with LStr str = {0} it sometimes tends to use the full aligned size to initialize memory and cause a GP.

The extra byte for the NULL byte indeed doesn't hurt and hardly counts. I sometimes do it too, as it allows me to pass the buffer directly to C functions that expect a C string pointer without having to first convert them. LabVIEW blissfully will ignore that extra byte at the end as long as the cnt value in the string is correct.

[j-medland]

Thanks @RolfKal. You are correct - I had muddled my mental model of how the alignment was working.

In the PR I submitted I encouraged them to use NumericArrayResize to allocate a string handle before passing it to Post event. They were already using that elsewhere in the codebase so resorting to malloc here was unnecessary.