tcunit/TcUnit

(W)STRING over 255 causes memory overwrite and page fault with AsssertEquals or is silently truncated by AssertEquals_(W)String

I-Campbell opened this issue · 4 comments

I-Campbell commented

With the current implementation, using a STRING or WSTRING with size over 255 with AssertEquals will overwrite memory, hopefully leading to an access violation.

TcUnit should not overwrite memory. Errors should be reported if unsuported input is used.

This could be solved by:

  1. implementing any length STRINGS/WSTRINGS.
  2. testing the size of memory to write at

    TcUnit/TcUnit/TcUnit/POUs/FB_TestSuite.TcPOU

    Line 2455 in 13276ed

    Tc2_System.MEMCPY(destAddr := ADR(stringExpected), srcAddr := Expected.pValue, n := DINT_TO_UDINT(Expected.diSize));
    and also lines 2456, 2463 and 2464. If unsupported input, generate error for the test.
  3. testing the size of memory as above, and silently only testing the supported length of 255, like how AssertEquals_STRING() works.

The following code shows the error:

METHOD Test1_1
VAR CONSTANT
	StringLength :DINT:= 1924;
	USE_WSTRING : BOOL := FALSE;
END_VAR
VAR
	a:WSTRING(StringLength):= "12";
	b:WSTRING(StringLength):= "12";	
	
	c:STRING(StringLength):= '12';
	d:STRING(StringLength):= '12';
END_VAR

TEST('Test1_1');

IF USE_WSTRING THEN 
	AssertEquals(Expected:= a, Actual:= b, Message:= 'No Mess');
ELSE
	AssertEquals(Expected:= c, Actual:= d, Message:= 'No Mess');
END_IF

TEST_FINISHED();
sagatowski commented

You're right, this should be handled. I would say you actually found two things here:

  1. Handling strings/wstrings with more than 255 characters using the AssertEquals(ANY)
  2. Should we continue to silently truncate strings/wstrings above 255 chars with AssertEquals_String/Wstring (I was actually not aware this happened)?

If we stick to only supporting 255 chars for now, the user should be notified that strings longer than 255 are not supported in both cases.

I-Campbell commented

Yes, a VAR_INPUT STRING(255) will truncate any string bigger than 255 that is passed to it.
This can be seen with the below test, which should FAIL, but in the current TcUnit code will PASS.

May I take on as a first 'code' issue, implementing unlimited length strings/wstrings?

I would need either VAR_IN_OUT CONSTANT STRING(16#7FFF_FFFE) or ANY_STRING or POINTER TO STRING

METHOD notEqualWstringsLarge

VAR CONSTANT
	StringLength :DINT:= 1984;
END_VAR
VAR
	a:WSTRING(StringLength):= "I am the very model of a modern Major-General,
I've information vegetable, animal, and mineral,
I know the kings of England, and I quote the fights historical
From Marathon to Waterloo, in order categorical;
I'm very well acquainted, too, with matters mathematical,
I understand equations, both the simple and quadratical,
About binomial theorem I'm teeming with a lot o' news,
With many cheerful facts about the square of the hypotenuse.";
	b:WSTRING(StringLength):= "I am the very model of a modern Major-General,
I've information vegetable, animal, and mineral,
I know the kings of England, and I quote the fights historical
From Marathon to Waterloo, in order categorical;
I'm very well acquainted, too, with matters mathemagical,
I understand equations, both the simple and quadratical,
About binomial theorem I'm teeming with a lot o' news,
With many cheerful facts about the square of the hypotenuse.";
END_VAR

TEST('notEqualWstringsLarge');

AssertEquals_WString(Expected:= a, Actual:= b, Message:= 'This should fail, as the Actual WSTRING mispelled mathematical');

TEST_FINISHED();
DavidHopkinsFbr commented

I don't think VAR_IN_OUT CONSTANT is supported in TC 4020. I don't know if that would prevent a .compiled-library built with 4022+ from working in a 4020 project?

POINTER TO STRING is perhaps best avoided because some organisations have coding standards which call for POINTER to be avoided in PLC code. So ANY_STRING is perhaps best (even though it's a still a pointer "under the hood"!). I think it is also quite semantically clear.

I'll just quickly note that there is a collection of slightly more obscure string functions in the Beckhoff libraries which support strings > 255 characters, unlike the standard functions, eg FIND2, CONCAT2, DELETE2, etc. Their APIs are based on pointers and buffer sizes. So there is an argument for using POINTER TO STRING in your design, in order to be consistent with the library extended string functions.

sagatowski commented

Another alternative is to simply add another assertion-function specific for strings > 255 chars, as to not brake current usage (basically the same way we have CONCAT and CONCAT2). The ANY-variant should handle > 255 chars, as it anyway already is using pointers (the ANY-type).