Menu

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

v1.0 (example)
closed-fixed
nobody
None
8
2019-05-26
2019-05-22
No

[I apologize for this submission being a duplicate of my posting into the discussion forum. I had not noticed the bug tracker facility before I posted. I will post an update to my forum message correcting this. Also, I assume a high priority number in my ticket submission means not urgent, which is why I picked 8 on the scale of 1-9, but please feel free to adjust it as you see fit, of course.]

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. 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. 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

1 Attachments

Discussion

  • Richard Barry

    Richard Barry - 2019-05-22
    • status: open --> closed-fixed
     
  • Adam Richter

    Adam Richter - 2019-05-26

    Hi, again, Richard.

    I am sorry that I made a serious typographical mistake in the patch I submitted, which inadvertently deleted an fwrite call FreeRTOS-Plus/Source/WolfSSL/wolfcrypt/test/test.c (at the end of the patch file). I updated my thread in the discussion forum about this, but I am worried that I did not get word to you, so I am posting to an updated to this ticket (which I did not earlier realize I could do). If you could acknoweldge that you have gotten this message when you get back from the Memorial Day weekend, such as by posting a reply here or in that discussion forum thread, I would be a relieve me of some fear that I have introduced a bug.

    Of course, I will try to be more careful about submitting patches in the futue..

    Thanks for all of your help with this.

     

Log in to post a comment.

MongoDB Logo MongoDB