Untested patches for a few cppcheck complaints (sprintf overrun, incorrect status bit message, trivial file leak)

adamjrichter wrote on Wednesday, May 22, 2019:

I want to pass along some untested possible fixes to a few problems that cppcheck found:

[FreeRTOS-Plus/Demo/FreeRTOS_Plus_TCP_Minimal_Windows_Simulator/DemoTasks/SimpleUDPClientAndServer.c:131]: (error) Buffer is accessed out of bounds: (char*)cString

[FreeRTOS-Plus/Source/FreeRTOS-Plus-TCP/portable/NetworkInterface/Common/phyHandling.c:575]: (error) Uninitialized variable: ulPHYLinkStatus

[FreeRTOS-Plus/Source/WolfSSL/wolfcrypt/test/test.c:3787]: (error) Resource leak: pemFile
[FreeRTOS-Plus/Source/WolfSSL/wolfcrypt/test/test.c:3932]: (error) Resource leak: pemFile

Please note that I am just trying cppcheck on various source trees, including FreeRTOS and have not built FreeRTOS, which is why these fixes are untested.

Here is a little more explanation about these possible fixes:

  1. In the Subversion tree, in the file FreeRTOS-Plus/Demo/FreeRTOS_Plus_TCP_Minimal_Windows_Simulator/DemoTasks/SimpleUDPClientAndServer.c, there is a function prvSimpleClientTask, which has a 50 byte string stack variable cString[] and does an sprintf to that string that is longer than 50 bytes (“Server received (not zero copy): Message number %lu\r\n”). The attached untested patch should fix the problem by increasing the string size to 60 bytes.

  2. The compaint about phyHandling.c is sort of a false positive. Some bits in the variable being tested are undefined, but the bit being tested is defined. However, it looks to me like the offending line and another one a few lines earlier were incorrectly testing a bit in the course of generating output text using “|=” instead of “&”, probably due to having copied some code that occurs a few lines earlier that sets the bit. I think the problem only effected human readable logging messages, as the variable in question is not used anywhere else. This change does not make the cppcheck complaint disappear, but I think it fixes a bug.

  3. The complaints about pemFile not being closed are about some error branches. It would be possible just to add fclose(pemFile) in the right places, but the success and failure branches for this test are basically the same, so this patch merges them to shrink the code slightly.

If there is anything else I should do to submit this patch for review and suggested for integration, I would appreciate knowing.

Thanks in advance for considering these changes.

Adam

adamjrichter wrote on Wednesday, May 22, 2019:

In case anyone wants to look at the other cppcheck complaints, and because it takes about 20 minutes to run “cppcheck --quiet .” on the whole source tree, I am also attaching the cppcheck output here, in case that encourages anyone to skim the output There are 427 complaints in this file, but, in my experience, the vast majority of cppcheck complaints turn out not to be bugs.

You can get more output from cppcheck by adding the “–force” argument (to try all ifdef combinations) and “–enable=all” to enable many more messages, including stylistic recommendations (which may also occasionally be triggered by a bug).

adamjrichter wrote on Wednesday, May 22, 2019:

Oops. Sorry. I did not realize that there is a separate bug tracker. I have resubmitted my patch there.

I remain subscribed to this thread, in case anyone wants to discuss another possible real bug (if any) in that cppcheck.log file or wants to share other analysis of it.

rtel wrote on Wednesday, May 22, 2019:

Thanks for these - I have updated the UDP demo and PHY handling files
(not checked in yet) and will report the WolfSSL test to the Wolf guys.
Appreciate you taking the time.

adamjrichter wrote on Wednesday, May 22, 2019:

Thanks for such a fast and careful response, Richard!

adamjrichter wrote on Wednesday, May 22, 2019:

Hi, again, Richard.

I am embarassed to write that I made a serious typographical error in the wolfcrypt change. I did not mean to delete or move the following line, toward the end of my patch, around line 3924 in FreeRTOS-Plus/Source/WolfSSL/wolfcrypt/test/test.c:

  •    ret = (int)fwrite(pem, 1, pemSz, pemFile);
    

I am sorry for my mistake. I will try to be more careful in my bug submissions here and elsewhere in the future.