Fix out of bounds read in formatString() on a trailing bare '%'

README.md

@@ -12,6 +12,41 @@ For more documentation and examples, see https://loupeteam.github.io/LoupeDocs/l
 # Installation
 To install using the Loupe Package Manager (LPM), in an initialized Automation Studio project directory run `lpm install stringext`. For more information about LPM, see https://loupeteam.github.io/LoupeDocs/tools/lpm.html
 
+# Self tests
+
+The example project in `example/As6Project` carries table-driven self tests for
+some of the library functions. Each is gated by a BOOL in the `Default` program:
+set it, and the test runs once on the next cycle and writes its results back.
+
+| Trigger | Covers | Results |
+|---|---|---|
+| `formatTest` | `formatString()` | `formatTestPass`, `formatTestFail`, `formatTestFirstFail` |
+| `hexTest` | `HexStringToUDINT()`, `UDINTToHexString()` | `hexTestPass`, `hexTestFail`, `hexTestFirstFail` |
+
+`...Fail` must come back 0. `...FirstFail` names the first input that did not
+behave as expected, and is left empty when the failure was a scalar assertion
+rather than a table case.
+
+Build and deploy the `Intel` configuration to ARsim, then set the trigger from
+the Automation Studio watch window, or over OPC UA from any client — the four
+variables of each suite are published in `OpcUaMap.uad`.
+
+The `Intel` configuration enables the OPC UA server on port 4840 and adds an
+unencrypted, anonymous endpoint, so that the self tests can be driven without
+putting a credential in this repository. The encrypted policies AS enables by
+default are still offered alongside it. Role `Everyone` is granted browse,
+read and write on the default view, so all eight variables above — results
+included, not just the triggers — are anonymously writable.
+
+That is appropriate for this project, which exists to compile the library and
+run its tests. **Do not copy this OPC UA configuration into a real machine.**
+
+The project pins OPC UA C/S 6.6.1, which is what the build runner carries.
+Publishing has been confirmed at runtime on 6.5.0 and 6.7.0; 6.6.1 has only
+been confirmed to build. 6.0.0 does **not** work — it compiles, and then the
+generated NodeSet fails to load with `BadNodeIdUnknown`, so nothing is
+published at all. If the variables do not appear, check this version first.
+
 ## Licensing
 
 This project is licensed under the [MIT License](LICENSE). 

example/As6Project/AsProject.apj

@@ -7,4 +7,7 @@
   <Motion RestartAcoposParameter="true" RestartInitParameter="true" />
   <Project StoreRuntimeInProject="false" />
   <Variables DefaultInitValue="0" DefaultRetain="false" DefaultVolatile="true" />
+  <TechnologyPackages>
+    <OpcUaCs Version="6.6.1" />
+  </TechnologyPackages>
 </Project>

example/As6Project/Logical/Programs/Default/Main.c

@@ -159,6 +159,284 @@ static void runHexTest(void)
 }
 
 
+/* Self test for formatString().
+	Set formatTest to run it; formatTestFail must come back 0.
+
+	The format of each case is copied into a scratch buffer whose remaining
+	bytes are filled with 'X' after the terminator. A formatter that walks
+	past the terminator - as a trailing bare '%' once did - drags that filler
+	into the destination, so the expected-output compare catches it. Only the
+	trailing '%' path can reach past the terminator at all: every other branch
+	consumes a character it has already seen to be non-null. */
+
+typedef struct formatCase_typ {
+	char*	Format;
+	char*	Expect;
+} formatCase_typ;
+
+static const formatCase_typ formatCases[] = {
+
+	/* Trailing bare '%' - no command character follows it */
+	{ "value: %",		"value: "	},
+	{ "%",				""			},
+	{ "trail %d%",		"trail 42"	},
+	{ "%%%",			"%"			},
+
+	/* Ordinary substitutions */
+	{ "no format",		"no format"	},
+	{ "%d items",		"42 items"	},
+	{ "%d and %d",		"42 and -7"	},
+	{ "%s=%b",			"abc=TRUE"	},
+
+	/* Escaped percent, and an unknown command character */
+	{ "100%% done",		"100% done"	},
+	{ "a%zb",			"ab"		},
+
+	/* Both halves of %b, and a null entry in the string array, which is
+		skipped without consuming a later argument in its place */
+	{ "%b%b",			"TRUEFALSE"	},
+	{ "%s|%s|%s",		"abc||xyz"	},
+
+	/* One more %d than the argument array holds - the sixth is dropped,
+		which is why the expected output ends in a space */
+	{ "%d %d %d %d %d %d",	"42 -7 0 0 0 "	},
+
+	/* The %s array runs out the same way, but through a second condition:
+		a null entry is skipped by one test and an exhausted array by another */
+	{ "%s%s%s%s%s%s",		"abcxyz"		},
+
+	/* %i is an alias for %d and is otherwise never exercised */
+	{ "%i and %i",			"42 and -7"		},
+
+};
+
+
+/* Cases that need a destination smaller than the formatted result. Same
+	scratch buffer treatment; DestSize is passed to formatString() verbatim. */
+
+typedef struct formatSizeCase_typ {
+	char*			Format;
+	unsigned long	DestSize;
+	char*			Expect;
+} formatSizeCase_typ;
+
+static const formatSizeCase_typ formatSizeCases[] = {
+
+	/* Trailing '%' reached with only the reserved null byte left. The loop
+		still has room to enter the '%' branch, so this is a genuine pre-fix
+		failure - the old code stepped onto the filler and copied it */
+	{ "abcdef%",	8,	"abcdef"	},
+
+	/* Destination fills before the format character is ever reached */
+	{ "abcdef%",	4,	"abc"		},
+
+	/* Only the reserved null byte fits - nothing is written at all */
+	{ "%",			1,	""			},
+
+	/* Truncation part way through a substitution. %b is the only branch
+		whose lengths are hard coded rather than computed, so it is the
+		likeliest place for a length bug and gets its own case. */
+	{ "%d",			2,	"4"			},
+	{ "%s",			3,	"ab"		},
+	{ "%b",			3,	"TR"		},
+
+	/* Escaped percent with exactly one byte to spare, and an exact fit */
+	{ "%%",			2,	"%"			},
+	{ "abcd",		5,	"abcd"		},
+
+};
+
+
+static void runFormatTest(void)
+{
+	unsigned long		i;
+	unsigned long		j;
+	StrExtArgs_typ		Args;
+	char				Scratch[64];
+	char				Dest[80];
+	char				Expected[64];
+	signed long			Length;
+	unsigned long		Overrun;
+
+	formatTestPass=	0;
+	formatTestFail=	0;
+	strcpy((char*)formatTestFirstFail, "");
+
+	for(i=0; i<(sizeof(formatCases)/sizeof(formatCases[0])); i++){
+
+		memset(&Args, 0, sizeof(Args));
+		Args.i[0]=	42;
+		Args.i[1]=	-7;
+		Args.b[0]=	1;
+		Args.s[0]=	(unsigned long)"abc";
+		Args.s[2]=	(unsigned long)"xyz";
+
+		/* Fill with junk, then lay the format in - everything after the
+			terminator stays 'X' and must never reach the destination. The
+			last byte is terminated so that a library that does run off the
+			end stops here rather than walking out of the buffer. */
+		for(j=0; j<sizeof(Scratch); j++) Scratch[j]= 'X';
+		Scratch[sizeof(Scratch)-1]= '\0';
+
+		/* A format that fills the scratch buffer would overrun it, and one
+			that nearly fills it leaves no filler to detect an over-read with */
+		if(strlen(formatCases[i].Format) >= (sizeof(Scratch) - 8)){
+
+			if(strlen((char*)formatTestFirstFail) == 0){
+				strncpy((char*)formatTestFirstFail, formatCases[i].Format, 79);
+			}
+
+			formatTestFail++;
+			continue;
+
+		}
+
+		strcpy(Scratch, formatCases[i].Format);
+
+		memset(Dest, 0, sizeof(Dest));
+
+		Length=	formatString(Dest, sizeof(Dest), Scratch, &Args);
+
+		if(		(strcmp(Dest, formatCases[i].Expect) == 0)
+			&&	(Length == (signed long)strlen(formatCases[i].Expect))
+			){
+
+			formatTestPass++;
+
+		}
+		else{
+
+			formatTestFail++;
+
+			if(strlen((char*)formatTestFirstFail) == 0){
+				strncpy((char*)formatTestFirstFail, formatCases[i].Format, 79);
+			}
+
+		}
+
+	}
+
+
+	/* Null pointers */
+
+	if( formatString(Dest, sizeof(Dest), "abc", 0) == STREXT_ERR_INVALID_INPUT ) formatTestPass++; else formatTestFail++;
+	if( formatString(0, sizeof(Dest), "abc", &Args) == STREXT_ERR_INVALID_INPUT ) formatTestPass++; else formatTestFail++;
+	if( formatString(Dest, sizeof(Dest), 0, &Args) == STREXT_ERR_INVALID_INPUT ) formatTestPass++; else formatTestFail++;
+
+
+	/* Destinations too small for the formatted result. Arguments are set up
+		explicitly here rather than inherited from the loop above, so these
+		cases do not change meaning if that table is ever edited. */
+
+	memset(&Args, 0, sizeof(Args));
+	Args.i[0]=	42;
+	Args.i[1]=	-7;
+	Args.b[0]=	1;
+	Args.s[0]=	(unsigned long)"abc";
+	Args.s[2]=	(unsigned long)"xyz";
+
+	for(i=0; i<(sizeof(formatSizeCases)/sizeof(formatSizeCases[0])); i++){
+
+		for(j=0; j<sizeof(Scratch); j++) Scratch[j]= 'X';
+		Scratch[sizeof(Scratch)-1]= '\0';
+
+		if(strlen(formatSizeCases[i].Format) >= (sizeof(Scratch) - 8)){
+
+			if(strlen((char*)formatTestFirstFail) == 0){
+				strncpy((char*)formatTestFirstFail, formatSizeCases[i].Format, 79);
+			}
+
+			formatTestFail++;
+			continue;
+
+		}
+
+		strcpy(Scratch, formatSizeCases[i].Format);
+
+		/* Fill the whole destination, so that anything written at or past
+			DestSize survives as a 'Z' and can be checked for below */
+		memset(Dest, 'Z', sizeof(Dest));
+
+		Length=	formatString(Dest, formatSizeCases[i].DestSize, Scratch, &Args);
+
+		/* Nothing may be written at or beyond DestSize */
+		Overrun=	0;
+		for(j=formatSizeCases[i].DestSize; j<sizeof(Dest); j++){
+			if(Dest[j] != 'Z') Overrun= 1;
+		}
+
+		if(		(strcmp(Dest, formatSizeCases[i].Expect) == 0)
+			&&	(Length == (signed long)strlen(formatSizeCases[i].Expect))
+			&&	(Overrun == 0)
+			){
+
+			formatTestPass++;
+
+		}
+		else{
+
+			formatTestFail++;
+
+			if(strlen((char*)formatTestFirstFail) == 0){
+				strncpy((char*)formatTestFirstFail, formatSizeCases[i].Format, 79);
+			}
+
+		}
+
+	}
+
+
+	/* A destination size of 0 must write nothing at all - not even the
+		terminator, since there is no byte to put it in */
+
+	memset(Dest, 'Z', sizeof(Dest));
+	if( (formatString(Dest, 0, "abc", &Args) == 0) && (Dest[0] == 'Z') ) formatTestPass++; else formatTestFail++;
+
+
+	/* Reals. The exact rendering belongs to brsftoa(), not to this test, so
+		compare against what brsftoa() itself produces for the same value */
+
+	Args.r[0]=	1.5;
+	brsftoa(Args.r[0], (unsigned long)Expected);
+
+	for(j=0; j<sizeof(Scratch); j++) Scratch[j]= 'X';
+	Scratch[sizeof(Scratch)-1]= '\0';
+	strcpy(Scratch, "%r");
+
+	memset(Dest, 0, sizeof(Dest));
+	if( (formatString(Dest, sizeof(Dest), Scratch, &Args) == (signed long)strlen(Expected))
+		&& (strcmp(Dest, Expected) == 0) ) formatTestPass++; else formatTestFail++;
+
+	for(j=0; j<sizeof(Scratch); j++) Scratch[j]= 'X';
+	Scratch[sizeof(Scratch)-1]= '\0';
+	strcpy(Scratch, "%f");
+
+	memset(Dest, 0, sizeof(Dest));
+	if( (formatString(Dest, sizeof(Dest), Scratch, &Args) == (signed long)strlen(Expected))
+		&& (strcmp(Dest, Expected) == 0) ) formatTestPass++; else formatTestFail++;
+
+
+	/* A real large enough to exercise the library's 16 byte scratch buffer,
+		which 1.5 leaves almost entirely untouched. Expected is deliberately
+		wider than that buffer: brsftoa() cannot be told how much room it has,
+		so the length has to be measured somewhere it cannot do damage, and
+		then asserted to fit where formatString() will put it. */
+
+	Args.r[0]=	-1.2345e38;
+	brsftoa(Args.r[0], (unsigned long)Expected);
+
+	for(j=0; j<sizeof(Scratch); j++) Scratch[j]= 'X';
+	Scratch[sizeof(Scratch)-1]= '\0';
+	strcpy(Scratch, "%r");
+
+	memset(Dest, 0, sizeof(Dest));
+	if( (strlen(Expected) < 16)
+		&& (formatString(Dest, sizeof(Dest), Scratch, &Args) == (signed long)strlen(Expected))
+		&& (strcmp(Dest, Expected) == 0) ) formatTestPass++; else formatTestFail++;
+
+}
+
+
 void _INIT ProgramInit(void)
 {
 }
@@ -182,6 +460,11 @@ void _CYCLIC ProgramCyclic(void)
 		runHexTest();
 	}
 
+	if(formatTest) {
+		formatTest = 0;
+		runFormatTest();
+	}
+
 
 
 }

example/As6Project/Logical/Programs/Default/Variables.var

@@ -10,4 +10,8 @@ VAR
 	hexTestPass : UDINT; (*Cases that behaved as expected*)
 	hexTestFail : UDINT; (*Cases that did not*)
 	hexTestFirstFail : STRING[80]; (*Input of the first case that failed*)
+	formatTest : BOOL; (*Rising edge runs the formatString self test*)
+	formatTestPass : UDINT; (*Cases that behaved as expected*)
+	formatTestFail : UDINT; (*Cases that did not*)
+	formatTestFirstFail : STRING[80]; (*Format of the first case that failed*)
 END_VAR

example/As6Project/Physical/ARM/X20CP0410/Connectivity/OpcUaCs/Package.pkg

@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="utf-8"?>
+<?AutomationStudio FileVersion="4.9"?>
+<Package SubType="OpcUaCs" PackageType="OpcUaCs" xmlns="http://br-automation.co.at/AS/Package">
+  <Objects>
+    <Object Type="File">UaCsConfig.uacfg</Object>
+  </Objects>
+</Package>

example/As6Project/Physical/ARM/X20CP0410/Connectivity/OpcUaCs/UaCsConfig.uacfg

@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Configuration>
+  <Element ID="ClientServerConfiguration" Type="uacfg">
+    <Property ID="OpcUaCs" Value="0" />
+  </Element>
+</Configuration>

example/As6Project/Physical/ARM/X20CP0410/Connectivity/Package.pkg

@@ -1,5 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <?AutomationStudio FileVersion="4.9"?>
 <Package SubType="Connectivity" PackageType="Connectivity" xmlns="http://br-automation.co.at/AS/Package">
-  <Objects />
+  <Objects>
+    <Object Type="Package">OpcUaCs</Object>
+  </Objects>
 </Package>