Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app_rpt: Incorrect usage of sizeof() function #207

Open
KB4MDD opened this issue Aug 14, 2023 · 3 comments · May be fixed by #473
Open

app_rpt: Incorrect usage of sizeof() function #207

KB4MDD opened this issue Aug 14, 2023 · 3 comments · May be fixed by #473
Assignees
Labels
bug Something isn't working

Comments

@KB4MDD
Copy link
Collaborator

KB4MDD commented Aug 14, 2023

The following functions appear to use the sizeof() function incorrectly.

apps/app_rpt/rpt_mdc1200.c
In mdc_1200_ack_status, line 209: memset(mdcp, 0, sizeof(&mdcp));

This should be sizeof(mdcparams)

apps/app_rpt/rpt_uchameleon.c
In uchameleon_connect, line 73: if ((count != 13) || (strncmp(expect, rxbuf + 4, sizeof(&expect)))) {

expect may need to be changed to a character array or change sizeof(&expect), or the literal 9, or strlen(expect).

Note: I am not sure why idbuf, ledbuf, and expect are declared as static. They probably should be char arrays and const.

@KB4MDD KB4MDD added the bug Something isn't working label Aug 14, 2023
@InterLinked1
Copy link
Member

The usage of sizeof looks fine to me. You could change the first one, line 209 to sizeof(struct mdcparams), but it would do the same thing it's doing now. The only reason to change it would be make it look more like the allocation on line 204.

These both look similar to the last issue Danny identified, where it's taking the address of a pointer. That is almost certainly not what is intended here. I think anything in the codebase with sizeof(& is probably highly suspect, and likely wrong.

The typical usage for this type of thing with a pointer is to use *, to dereference, rather than &, which does the opposite, and gets the address of the variable. Taking the sizeof of a pointer in general will just give the size of a pointer (4 or 8), which is also wrong. Given the variable is a pointer, this gets the address of the pointer. Now add sizeof and you have the size of a pointer on your system. 100% not what we want.

In contrast, using * instead is very common and correct.

As discussed before, sizeof should not be used directly for strlen. The usage is wrong and confusing. If the string is null terminated, I don't see why strcmp could not be used.

@encbar5
Copy link
Collaborator

encbar5 commented Aug 14, 2023

Ah yes, I swapped those in my head.

@Allan-N
Copy link
Collaborator

Allan-N commented Dec 6, 2024

The sizeof() usage that @KB4MDD called out should be fixed.

To see the issue, compile the following :

#include <stdio.h>

struct mdcparams
{
	char    type[10];
	short   UnitID;
	short   DestID;
	short   subcode;
};

int
main(int argc, char **argv)
{
	struct mdcparams *mdcp;
	printf("sizeof(&mdcp) = %zu\n", sizeof(&mdcp));
	printf("sizeof(*mdcp) = %zu\n", sizeof(*mdcp));
	printf("sizeof(struct mdcparams) = %zu\n", sizeof(struct mdcparams));
}

and when exec'd you'll get :

sizeof(&mdcp) = 8
sizeof(*mdcp) = 16
sizeof(struct mdcparams) = 16

Using "sizeof(&mdcp)" is clearly not correct (it's the size of an address vs. the size of the allocation).

The usage of sizeof(&expect) is also incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants