libscript: Tweak MCScriptForeignInvocation to help Coverity

peter-b · peter-b · commit 9ff496eae089 · 2016-10-24T18:03:06.000+01:00
The `MCScriptForeignInvocation` implementation is confusing Coverity,
which is reporting a bunch of uninitialised memory errors relating to
pointers acquired via the `MCScriptForeignInvocation::Allocate()`
method.  There were two possible problems:

- The maximum storage allocatable was stored in
  `MCScriptForeignInvocation::m_storage_limit`, a mutable member
  variable (despite being a compile-time constant).  It's possible
  that Coverity was assuming that it could be mutated at some point to
  be larger than the size of the fixed-size `m_stack_storage` array,
  allowing `Allocate()` to return a pointer to invalid memory.  This
  patch removes the member.

- `MCScriptForeignInvocation::ReferenceArgument()` and `Argument()`
  were checking whether the current argument count was _equal_ to the
  maximum number of arguments, rather than checking that it was less
  than the maximum.  This could allow Coverity to assume that the
  current argument count was greater than the maximum number of
  arguments, once again allowing pointers to invalid memory to be
  produced.  This patch introduces inequalities.

Coverity-Id: 137925
Coverity-Id: 137927
diff --git a/libscript/src/script-execute.cpp b/libscript/src/script-execute.cpp
@@ -41,8 +41,7 @@ class MCScriptForeignInvocation
 public:
 	MCScriptForeignInvocation(void)
 		: m_argument_count(0),
-		  m_storage_frontier(0),
-		  m_storage_limit(sizeof(m_stack_storage))
+		  m_storage_frontier(0)
 	{
 	}
 	
@@ -71,7 +70,7 @@ class MCScriptForeignInvocation
 		t_align_delta = p_align - (m_storage_frontier % p_align);
 		
 		// If there is not enough space then throw.
-		if (m_storage_limit - m_storage_frontier < p_amount + t_align_delta)
+		if (sizeof(m_stack_storage) - m_storage_frontier < p_amount + t_align_delta)
 		{
 			return MCErrorThrowOutOfMemory();
 		}
@@ -92,7 +91,7 @@ class MCScriptForeignInvocation
 	bool Argument(void *p_slot_ptr,
 				  __MCScriptValueDrop p_slot_drop)
 	{
-		if (m_argument_count == kMaxArguments)
+		if (m_argument_count >= kMaxArguments)
 		{
 			return MCErrorThrowOutOfMemory();
 		}
@@ -114,7 +113,7 @@ class MCScriptForeignInvocation
 	bool ReferenceArgument(void *p_slot_ptr,
 						   __MCScriptValueDrop p_slot_drop)
 	{
-		if (m_argument_count == kMaxArguments ||
+		if (m_argument_count >= kMaxArguments ||
 			!Allocate(sizeof(void *),
 					  alignof(void *),
 					  m_argument_values[m_argument_count]))
@@ -180,7 +179,6 @@ class MCScriptForeignInvocation
 	void *m_argument_slots[kMaxArguments];
 	
 	size_t m_storage_frontier;
-	size_t m_storage_limit;
 	char m_stack_storage[kMaxStorage];
 };
 

Original file line number	Diff line number	Diff line change
`@@ -41,8 +41,7 @@ class MCScriptForeignInvocation`
`41`	`41`	`public:`
`42`	`42`	`MCScriptForeignInvocation(void)`
`43`	`43`	`: m_argument_count(0),`
`44`		`- m_storage_frontier(0),`
`45`		`- m_storage_limit(sizeof(m_stack_storage))`
	`44`	`+ m_storage_frontier(0)`
`46`	`45`	`{`
`47`	`46`	`}`
`48`	`47`
`@@ -71,7 +70,7 @@ class MCScriptForeignInvocation`
`71`	`70`	`t_align_delta = p_align - (m_storage_frontier % p_align);`
`72`	`71`
`73`	`72`	`// If there is not enough space then throw.`
`74`		`- if (m_storage_limit - m_storage_frontier < p_amount + t_align_delta)`
	`73`	`+ if (sizeof(m_stack_storage) - m_storage_frontier < p_amount + t_align_delta)`
`75`	`74`	`{`
`76`	`75`	`return MCErrorThrowOutOfMemory();`
`77`	`76`	`}`
`@@ -92,7 +91,7 @@ class MCScriptForeignInvocation`
`92`	`91`	`bool Argument(void *p_slot_ptr,`
`93`	`92`	`__MCScriptValueDrop p_slot_drop)`
`94`	`93`	`{`
`95`		`- if (m_argument_count == kMaxArguments)`
	`94`	`+ if (m_argument_count >= kMaxArguments)`
`96`	`95`	`{`
`97`	`96`	`return MCErrorThrowOutOfMemory();`
`98`	`97`	`}`
`@@ -114,7 +113,7 @@ class MCScriptForeignInvocation`
`114`	`113`	`bool ReferenceArgument(void *p_slot_ptr,`
`115`	`114`	`__MCScriptValueDrop p_slot_drop)`
`116`	`115`	`{`
`117`		`- if (m_argument_count == kMaxArguments \|\|`
	`116`	`+ if (m_argument_count >= kMaxArguments \|\|`
`118`	`117`	`!Allocate(sizeof(void *),`
`119`	`118`	`alignof(void *),`
`120`	`119`	`m_argument_values[m_argument_count]))`
`@@ -180,7 +179,6 @@ class MCScriptForeignInvocation`
`180`	`179`	`void *m_argument_slots[kMaxArguments];`
`181`	`180`
`182`	`181`	`size_t m_storage_frontier;`
`183`		`- size_t m_storage_limit;`
`184`	`182`	`char m_stack_storage[kMaxStorage];`
`185`	`183`	`};`
`186`	`184`