Source code review

OSTC running hwOS4 Firmware
Post Reply
Posts: 10
Joined: Saturday 29. September 2018, 11:23

Source code review

Post by thomas.horner »

In file Discovery/Src/gfx_engine.c in line 3037 there's a possible null pointer dereference as two lines above, in line 3035, the variable it explicitly set to zero (NULL pointer, if you want). Line 3037 is missing a check for pText != 0

In file Discovery/Src/gfx_engine.c in line 3159 the variable decodeUTF8 might be uninitialized if((*(char*)pText == '\005') || (*(char*)pText == '\006')). I guess you could init decodeUTF with pText (instead of line 3156) already in line 3143 and remove the else branch completely.

In file Discovery/Src/simulation.c in line 202 (and the two lines below) the used variable localCalibCoeff might not have been initialized before and might contain a random value.
This is because it's not initialized to zero when defining the variable and it's not initialized if the variable checkOncePerSecond happens to have the value "false" - see line 137 "if(checkOncePerSecond)".

In file Discovery/Src/tMenuEditPlanner.c on line 307 the function strncpy is used. The function strncpy doesn't make sure the target string properly ends with zero. The const string currently copied has less chars than the 40 to be copied, so that it should be expected that strncpy currently "by chance" properly copies the required zero terminator from the const string. On future changes however, there's quite some risk of counting "to the exact lenght" and hence not terminating the string. The next line contains write_label_var which in turn calls GFX_write_string_color which relies on the string being properly terminated. It would be wise to use the function strcpy when copying from a const string as that will make sure to copy the terminating zero.

"Only" a tool, but despite that:
In file ostc4pack/src/OSTC4pack_V4.cpp the file-handle is closed in line 443 but
in line 459 the content of buf2 should be written to it,
in line 499 the content of buf3offset should be written to it,
... in lines 502, 504, 518.
As the file-handle fpout is opened in line 458 i assume it was meant to be used instead of fp:
fpout = fopen(filenameout, "wb");
Maybe the definition of fpout should be removed as anyway not both file handles are used at the same time. Then fp could be reused for output:
fp = fopen(filenameout, "wb");
Gcc let's me know that: ‘sprintf’ output between 12 and 511 bytes into a destination of size 510
So it's save to assume that filenameout[510] must be corrected to filenameout[511].
The last warnings in this file tell us that variables of size_t are provided where an int is expected.
printf("%d bytes read (hex: %#x )\n", len, len);
To build warning free, all that's needed to do is add a cast, e.g. to uint32_t:
printf("%d bytes read (hex: %#x )\n", (uint32_t)len, (uint32_t)len);
The file-handle fpout is never properly closed, it seems.
I have attached including the updated sources and for easy building.
Be aware that as I have allegedly "fixed" the fpout issue, the output might be different to what it was before and - refusing to install eclipse for solely this project - I couldn't yet verify if the output is better or worse than before.
(8.74 KiB) Downloaded 1543 times
Post Reply