Possibly unintended write to inBuf in codeplugUtilConvertBufToString()

[OH2CBF]

While looking into the code, to understand why the contact name has stopped showing up while receving in DMR mode, I noticed that in function "void codeplugUtilConvertBufToString(...)" the buffer to which inBuf points to gets written to. Seems like this might be unintentional and could cause some unexpected side effects.

https://github.com/rogerclarkmelbourne/ ... lug.c#L196

Code: Select all

void codeplugUtilConvertBufToString(char *inBuf, char *outBuf, int len)
{
	for(int i = 0; i < len; i++)
	{
		if (inBuf[i] == 0xff)
		{
			inBuf[i] = 0;    <---- here
		}
		outBuf[i] = inBuf[i];
	}
	outBuf[len] = 0;
	return;
}

[F1RMB]

Hi,

I don't see the problem here. 0xFF byte value in the codeplug is "NULL".


Cheers.
---
Daniel

[F1RMB]

About your problem, it would be better to explain what is going on (by contact, do you mean codeplug contact, or DRMid database ?).

Cheers.
---
Daniel

[OH2CBF]

Hi Daniel

I don't think my problem is related to the issue I reported. Writing back to memory referenced with pointer called "input" just seemed to me like a mistake which could cause some unexpected results. Anyway, I am not familiar with your codebase, and if that is intended, just ignore my report. My original issue is probably caused by something I did when importing the DMR ID database from CSV file. That just made me look into the code. I'll make another report about that if I think there's something wrong.

Thanks for your reply,
Ville

[F1RMB]

Hi Ville,

Hi Daniel

I don't think my problem is related to the issue I reported. Writing back to memory referenced with pointer called "input" just seemed to me like a mistake which could cause some unexpected results. Anyway, I am not familiar with your codebase, and if that is intended, just ignore my report. My original issue is probably caused by something I did when importing the DMR ID database from CSV file. That just made me look into the code. I'll make another report about that if I think there's something wrong.

Thanks for your reply,
Ville

Okay. Yes, checkout your codeplug, re-upload the DMRIds, just to be sure every condition is under control.


Cheers.
---
Daniel

[VK3KYY]

This is not a mistake.

The codeplug format does not use the standard string terminator of 0, it uses 0xFF

The only potential improvement would be to rename the function argument 'inBuf' to 'codeplugBuf'

The function could be rewritten to not change the input buffer (see below)
However, I don't know whether the resultant assembler / binary code would be larger and slower.

I see the function has a return statement, which is unnecessary in a function that returns void, but I'm sure the compiler is smart enough to ignore it.

With firmware, often come optimisations are made, which you would not bother with on a PC etc.
Also note that neither of the arguments are declared const, hence why it does give a compile error.

I don't have time to check the assembly output from this to see whether adding an else makes the code bigger and slower, but we could change the argument name of inBuf to codeplugBuf.

Code: Select all

void codeplugUtilConvertBufToString(char *inBuf, char *outBuf, int len)
{
	for(int i = 0; i < len; i++)
	{
		if (inBuf[i] != 0xff)
		{
			outBuf[i] = inBuf[i];
  
		}
		else
		{
			 outBuf[i] = 0;  
		}
	}
	outBuf[len] = 0;
	return;
}

Or we could look at the code to see if we can just pass one buffer and get it to change that buffer i.e inBuf

I suspect this would be faster and smaller

Code: Select all

void codeplugUtilConvertBufToString(char *inBuf,int len)
{
char *tmpBuf = inBuf;
	inBuf[len] = 0;
	do
	{
		if (*tmpBuf  == 0xff)
		{
			 tmpBuf = 0;  
		}
	} while(*tmpBuf++ !=0);
}

However, looking at where this function was used (and its used a lot), it would require a lot of changes to the code, and potentially add bugs whilst things were modified, so its probably not worth the effort.

[OH2CBF]

This is not a mistake.

Right, that's basically all I wanted to confirm.

(I got thinking about the different ways the code could backfire, ie. calling the function twice with the same inBuf ptr, etc, but I didn't check all the calls of the function in the code.)

The codeplug format does not use the standard string terminator of 0, it uses 0xFF

Got it, that's why you use the conversion in the first place

The function could be rewritten to not change the input buffer (see below)
However, I don't know whether the resultant assembler / binary code would be larger and slower.

I doubt it would make any meaningful difference for size or performance.

I see the function has a return statement, which is unnecessary in a function that returns void, but I'm sure the compiler is smart enough to ignore it.

Return is implied at the end of the function block anyway (call stack is unwound, execution is returned to the caller). It's valid to write the return explicitly at the end or leave it out. It has no effect in a void returning function. Return statement can be of course useful also in void function() to get out in the middle of function, for example in a loop, when a condition is met.

With firmware, often come optimisations are made, which you would not bother with on a PC etc.

Yes, naturally

The first snippet in you your reply is close to what I would have expected to see when my suspicion of a mistake crept. One could use the return from mid loop, mentioned above, here too:

Code: Select all

void codeplugUtilConvertBufToString(const char *inBuf, char *outBuf, int len)
{
        /* warning: outBuf is written even if len == 0 or negative */

	int i;

        for(i = 0; i < len; i++)
        {
                if (inBuf[i] == 0xff)
                {
                        outBuf[i] = 0;
                        return;                  <--- Here
                }
                outBuf[i] = inBuf[i];
        }
        outBuf[i] = 0;
        return;
}

(But leaving the example aside, you would really rather just

Code: Select all

if(inBuf[i] == 0xff) break;

instead, in this particular case.)

For the second snippet of your reply, with my simple mind, I find it hard to reason about. Personally, I'd prefer the first one, for what that's worth.

Another side note for the second one (which you probably are well aware of), duplicating the buffer pointer is not required. You would have to use **buf to be able to modify the callers pointer. The pointer is copied when used as a function parameter and modifying it inside the function has only local effect. Just wanted to mention this, because it's often overlooked. You could probably formulate it like this to make it minimalistic:

Code: Select all

void CodePlugBufToString(char *plugBuf, int maxLen)
{
        /* same warning applies as for the previous one */

        int i = 0;

        while( i < maxLen && plugBuf[i] != 0xff ) i++;

        plugBuf[i] = 0;
}

Anyway, I think I have wasted enough of your time.

I fully understand that not everything worth doing is worth doing perfectly. One does not have infinitely time. I am grateful to the past and current developers for the work on the project. I love to be able to see what's actually going on instead of just staring at the black box.

Cheers,
Ville