#39 Assert_Equals() with STRING(1024) Causes PLC Crash

[FUTURE FEATURE]
pending
nobody
None
2020-06-17
2020-05-01
i-campbell
No

Test version: v1.0.9.9 RC2
Problem: Using Assert_Equals() with STRING(1024) Causes PLC to crash.
The problem is, the AssertEquals uses pointers to write to memory, and will happily copy an entire 1024 character string over the top of a 255 character string
See screenshot for the line which causes the memory overwrite
Steps to repeat:
With the Verifier_MultiCycle_Examplev1.0.9.9.project, change the following POU:
Test/FB_AnyPrimitiveTypes.Test_ANY_STRING_Equals
to

METHOD PRIVATE Test_ANY_STRING_Equals
VAR
    a : STRING(1024) := 'WCPRZC2VAB
01SQ7HD707
68UUBLAN0H
AI2HPVN8YO
NQ5T9PIV9M
DAQYA96Y8T';
    b : STRING(1024) := 'WCPRZC2VAB
01SQ7HD707
68UUBLAN0H
AI2HPVN8YO
NQ5T9PIV9M
DAQYA96Y8T';
END_VAR
TEST('Test_ANY_STRING_Equals');

AssertEquals(Expected := a,
             Actual := b,
             Message := 'Values differ');

TEST_FINISHED();

Download to the PLC and run
==> IS: PLC stops unexpectedly, no logs written, PLC won't start again
==> SHOULD: Either allow to compare any length strings, OR don't allow them and print an error message rather than crashing the PLC
Note: To recover, delete the application from the PLC, for example at C:\ProgramData\CODESYS\CODESYSControlWinV3x64\AAAA\PlcLogic\Application\

1 Attachments

Discussion

  • aliazzz

    aliazzz - 2020-05-01

    Hi,

    Sadly at this moment, string(1024) is not supported, so technically it is not a Bug, but a feature ;-)
    Currently a string with maximum length of 255 ( type T_MaxString) is supported only. I will put this idea on hold for the future as it might be a good idea to test larger strings. This idea has to wait for the next release beyond V1.1.0.0.

    Hope this helps

    Aliazzz

     

    Last edit: aliazzz 2020-05-01
  • aliazzz

    aliazzz - 2020-05-01
    • status: unread --> pending
     
  • aliazzz

    aliazzz - 2020-05-01
    • Milestone: v1.1.0.0 --> [FUTURE]
     
  • aliazzz

    aliazzz - 2020-05-01

    On a second thought, adding a test for the stringlength might be a good idea to prevent the runtime from crashing.
    The idea is to clip off any extra characters beyond 255. This way the assert stays true to its intent without causing larger content to crash the runtime

    A future Extension possibility would be to add a new assert function to accept WSTRING or lengths beyond 255 chars.

     
    • i-campbell

      i-campbell - 2020-05-01

      Truncating is unfair, as an Assert value that differes only after byte 255 will return SUCCESS when it should return NOT_IMPLEMENTED. I would rather cfUnit crash my test PLC than just compare some of it.

      Changing line 147 and 148 to

      (* STRING <= STRING(255) *)
          IF UDINT_TO_INT(Expected.TypeClass) = IBaseLibrary.TypeClass.TYPE_STRING  AND (Expected.diSize <= SIZEOF(T_MaxString)) AND (Actual.diSize <= SIZEOF(T_MaxString)) THEN
      

      Will cause it to fall through to the "compare byte by byte" stuff at the bottom (from line 213).
      This is good, but will fall down when comparing strings that have not initialized the bytes after the 0x00 Null terminating character.

       

      Last edit: i-campbell 2020-05-01
  • i-campbell

    i-campbell - 2020-05-01
    Thoughts for implementing STRING > 255

    You already have the IECStringUtils library in the project.
    So it should be OK to Use:

    // Will return 0 if STRINGs are equal
    IECStringUtils.Stu.StrCmpA(Expected.pValue, Actual.pValue);
    

    If they are not equal, AssertEquals.IteratorCounter will already hold the Position where they first differ. It may be prudent to log:
    'Strings first differ at character: ' TO_STRING(IteratorCounter)
    And then only LOG ~50 characters before and ~50 characters after the change. Make sure to limit this to within the range of the STRING

     
  • aliazzz

    aliazzz - 2020-06-17

    Hi Ian,

    Would you mind to give an update on the status of this Ticket?

    Thank you!

     
  • i-campbell

    i-campbell - 2020-06-17
    1. TcUnit AssertEquals() still overwrites memory
    2. cfUnit RC5 AssertEquals() still overwrites memory
    3. Additionally, AssertEquals_WSTRING() and AssertEquals_STRING() happily accept strings over 255 in length, and just ignore any characters after the 255th. This might be obvious to the user though, as the function asks for two STRING(255)s.
    4. I have a workaround WstringEquals (FUN) / StringEquals (FUN) with AssertEquals_BOOL here https://forge.codesys.com/u/i-campbell/comparestrings/ci/master/tree/CompareStrings.library

    So I probably need to learn the TcUnit pull request technique, develop and submit a pull request in TcUnit?
    or just submit a pull request to cfUnit?

     

    Last edit: i-campbell 2020-06-17

Log in to post a comment.