Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
I-Campbell opened this issue May 31, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@I-Campbell
Copy link
Contributor

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
    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
Copy link
Member

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.

@sagatowski sagatowski added the enhancement New feature or request label Jun 9, 2020
@I-Campbell
Copy link
Contributor Author

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();

@I-Campbell I-Campbell changed the title AssertEquals STRING or WSTRING over 255 causes memory overwrite and page fault (W)STRING over 255 causes memory overwrite and page fault with AsssertEquals or is silently truncated by AssertEquals_(W)String Jun 17, 2020
@DavidHopkinsFbr
Copy link
Contributor

DavidHopkinsFbr commented Jul 6, 2020

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants