Fix many allocation/deallocation mismatches.

peter-b · peter-b · commit 90de18b81742 · 2016-07-08T11:17:22.000+01:00
The LiveCode engine uses three different allocators with distinct
semantics.

- `malloc()`/`free()`: these are C library procedures, usually
  accessed via the `MCMemory*` libfoundation functions; they allocate
  memory on the heap and do no C++ construction or destruction of
  their contents.

- `new`/`delete`: these C++ operators allocate/deallocate a single
  instance on the heap, and calls its constructor/destructor.

- `new[]`/`delete[]`: these C++ operators allocate/deallocate an sized
  array of instances on the heap, and call the constructor/destructor
  for each of them.

Mismatch (e.g. allocating with `new[]` and freeing with `free()` is a
resource management error, since it can result in something having a
destructor called when it shouldn't (or vice versa).  Both Coverity
Scan and the newest version of valgrind detect mismatched use.

This patch is a mostly mechanical replacement of release statements to
match the corresponding allocators, with addition of annotation of
what the expected allocator is.  (I added the annotation to help me
detect when multiple codepaths were allocating in conflicting
ways). As part of this, I changed a couple of inconsistent allocation
paths.

A particular error that occurs frequently is using an
`MCAutoPointer&lt;T&gt;` to manage a `T*` that was created using `malloc()`;
in particular, the usual error is to try and put the result of a
libfoundation function that allocates a buffer into an
`MCAutoPointer&lt;T&gt;`.  This won't work, because libfoundation always
allocates memory with `malloc()`, and `MCAutoPointer` always destroys
its contents with `delete`.  Several codepaths are corrected to use an
appropriate `MCAutoCustomPointer` instead.
diff --git a/engine/src/aclip.cpp b/engine/src/aclip.cpp
@@ -154,7 +154,7 @@ MCAudioClip::~MCAudioClip()
 		stop(True);
 		MCacptr = NULL;
 	}
-	delete samples;
+	delete[] samples; /* Allocated with new[] */
 	delete osamples;
 #ifdef TARGET_PLATFORM_LINUX
 	if ( x11audio != NULL )
diff --git a/engine/src/answer.cpp b/engine/src/answer.cpp
@@ -65,7 +65,7 @@ MCAnswer::~MCAnswer()
 			delete file . filter;
 			for(uint4 t_type = 0; t_type < file . type_count; ++t_type)
 				delete file . types[t_type];
-			delete file . types;
+			delete[] file . types; /* Allocated with new[] */
 		break;
 		
 		case AT_FOLDER:
@@ -78,7 +78,7 @@ MCAnswer::~MCAnswer()
 			delete notify . prompt;
 			for(uint4 t_button = 0; t_button < notify . button_count; ++t_button)
 				delete notify . buttons[t_button];
-			delete notify . buttons;
+			delete[] notify . buttons; /* Allocated with new[] */
 		break;
 	}
 }
diff --git a/engine/src/ask.cpp b/engine/src/ask.cpp
@@ -67,7 +67,7 @@ MCAsk::~MCAsk(void)
 		delete file . filter;
 		for(uint4 t_type = 0; t_type < file . type_count; ++t_type)
 			delete file . types[t_type];
-		delete file . types;
+		delete[] file . types; /* Allocated with new[] */
 	break;
 
 	default:
diff --git a/engine/src/dispatch.cpp b/engine/src/dispatch.cpp
@@ -178,8 +178,8 @@ MCDispatch::~MCDispatch()
 	}
 	delete fonts;
 	
-	delete startdir;
-	delete enginedir;
+	MCMemoryDeleteArray(startdir); /* Allocated by MCStringConvertToCString() */
+	MCMemoryDeleteArray(enginedir); /* Allocated by MCStringConvertToCString() */
 	
 	delete m_externals;
     // AL-2015-02-10: [[ Standalone Inclusions ]] Delete library mapping
diff --git a/engine/src/dsklnx.cpp b/engine/src/dsklnx.cpp
@@ -1395,7 +1395,7 @@ class MCLinuxDesktop: public MCSystemInterface
             }
         }
 
-		delete t_entry_path;
+        delete[] t_entry_path; /* Allocated with new[] */
         closedir(dirptr);
 
         return t_success;
diff --git a/engine/src/eps.cpp b/engine/src/eps.cpp
@@ -98,8 +98,8 @@ MCEPS::MCEPS(const MCEPS &sref) : MCControl(sref)
 
 MCEPS::~MCEPS()
 {
-	delete postscript;
-	delete prolog;
+	delete[] postscript; /* Allocated with new[] */
+	delete[] prolog; /* Allocated with new [] */
 	delete pageIndex;
 	delete image;
 }
diff --git a/engine/src/exec-array.cpp b/engine/src/exec-array.cpp
@@ -961,7 +961,7 @@ bool MCArraysCopyExtents(MCArrayRef self, array_extent_t*& r_extents, uindex_t&
 
 	while (MCArrayIterate(self, t_index, t_key, t_value))
 	{
-		MCAutoPointer<index_t> t_indexes;
+		MCAutoCustomPointer<index_t,MCMemoryDeleteArray> t_indexes;
 		uindex_t t_index_count;
 		bool t_all_integers;
 		if (!MCArraysSplitIndexes(t_key, &t_indexes, t_index_count, t_all_integers))
diff --git a/engine/src/exec-interface-stack.cpp b/engine/src/exec-interface-stack.cpp
@@ -1485,7 +1485,7 @@ void MCStack::SetStackFiles(MCExecContext& ctxt, MCStringRef p_files)
 		MCValueRelease(stackfiles[nstackfiles].stackname);
 		MCValueRelease(stackfiles[nstackfiles].filename);
 	}
-	delete stackfiles;
+	delete[] stackfiles; /* Allocated with new[] */
 
     if (stringtostackfiles(p_files, &stackfiles, nstackfiles))
         return;
diff --git a/engine/src/exec-interface2.cpp b/engine/src/exec-interface2.cpp
@@ -564,7 +564,7 @@ void MCInterfaceStackFileVersionParse(MCExecContext& ctxt, MCStringRef p_input,
 	char *t_version;
 	/* UNCHECKED */ MCStringConvertToCString(p_input, t_version);
     count = sscanf(t_version, "%d.%d.%d", &major, &minor, &revision);
-	delete t_version;
+    MCMemoryDeleteArray(t_version);
 	
 	version = major * 1000 + minor * 100 + revision * 10;
 	
diff --git a/engine/src/externalv0.cpp b/engine/src/externalv0.cpp
@@ -353,9 +353,9 @@ Exec_stat MCExternalV0::Handle(MCObject *p_context, Handler_type p_type, uint32_
     if (args != NULL)
     {
         while (nargs--)
-            delete args[nargs];
+	        MCMemoryDeleteArray(args[nargs]); /* Allocated with MCStringNormalizeAndConvertToCString */
         
-        delete args;
+        delete[] args; /* Allocated with new[] */
     }
     
     if (Xerr)
diff --git a/engine/src/field.cpp b/engine/src/field.cpp
@@ -370,7 +370,7 @@ MCField::~MCField()
 	else
 		delete vscrollbar;
 
-	delete tabs;
+	delete[] tabs; /* Allocated with new[] */
 
 	MCValueRelease(label);
 }
diff --git a/engine/src/globals.cpp b/engine/src/globals.cpp
@@ -1297,9 +1297,9 @@ int X_close(void)
 	while (MCnfiles)
 		IO_closefile(MCfiles[0].name);
 	if (MCfiles != NULL)
-		delete MCfiles;
+		delete[] MCfiles; /* Allocated with new[] */
 	if (MCprocesses != NULL)
-		delete MCprocesses;
+		delete[] MCprocesses; /* Allocated with new[] */
 	if (MCnsockets)
 		while (MCnsockets)
 			delete MCsockets[--MCnsockets];
@@ -1359,7 +1359,7 @@ int X_close(void)
 	delete MCdialogdata;
 	MCValueRelease(MChcstat);
 
-	delete MCusing;
+	delete[] MCusing; /* Allocated with new[] */
 	MCValueRelease(MChttpheaders);
 	MCValueRelease(MCscriptfont);
 	MCValueRelease(MClinkatts . colorname);
@@ -1499,7 +1499,7 @@ int X_close(void)
 	MCU_finalize_names();
 	
 	if (MCsysencoding != nil)
-		MCMemoryDelete(MCsysencoding);
+		delete[] MCsysencoding; /* Allocated with new[] */
 
     if (kMCSystemLocale != nil)
         MCLocaleRelease(kMCSystemLocale);
diff --git a/engine/src/gradient.cpp b/engine/src/gradient.cpp
@@ -708,7 +708,7 @@ IO_stat MCGradientFillUnserialize(MCGradientFill *p_gradient, MCObjectInputStrea
 	if (t_stat == IO_NORMAL)
 	{
 		if (p_gradient != NULL)
-			delete p_gradient -> ramp;
+			delete[] p_gradient -> ramp; /* Allocated with new[] */
 		p_gradient -> ramp = new MCGradientFillStop[p_gradient -> ramp_length];
 		for(int i = 0; i < p_gradient -> ramp_length && t_stat == IO_NORMAL; ++i)
 		{
diff --git a/engine/src/graphic.cpp b/engine/src/graphic.cpp
@@ -200,9 +200,9 @@ void MCGraphic::initialise(void)
 
 void MCGraphic::finalise(void)
 {
-	delete dashes;
-	delete points;
-	delete realpoints;
+	delete[] dashes; /* Allocated with new[] */
+	delete[] points; /* Allocated with new[] */
+	delete[] realpoints; /* Allocated with new[] */
 	delete markerpoints;
 	delete oldpoints;
 	MCValueRelease(label);
diff --git a/engine/src/handler.cpp b/engine/src/handler.cpp
@@ -86,20 +86,20 @@ MCHandler::~MCHandler()
 		MCNameDelete(vinfo[i] . name);
 		MCValueRelease(vinfo[i] . init);
 	}
-	delete vinfo;
+	delete[] vinfo; /* Allocated with new[] */
 
 	for(uint32_t i = 0; i < npnames; i++)
 		MCNameDelete(pinfo[i] . name);
-	delete pinfo;
+	delete[] pinfo; /* Allocated with new[] */
 
-	delete globals;
+	delete[] globals; /* Allocated with new[] */
 
 	for(uint32_t i = 0; i < nconstants; i++)
 	{
 		MCNameDelete(cinfo[i] . name);
 		MCValueRelease(cinfo[i] . value);
 	}
-	delete cinfo;
+	delete[] cinfo; /* Allocated with new[] */
 	
 	// MW-2013-11-08: [[ RefactorIt ]] Delete the it varref.
 	delete m_it;
@@ -523,7 +523,7 @@ Exec_stat MCHandler::exec(MCExecContext& ctxt, MCParameter *plist)
                 delete params[i];
             }
         }
-		delete params;
+		delete[] params; /* Allocated with new[] */
 	}
 	if (vars != NULL)
 	{
@@ -536,7 +536,7 @@ Exec_stat MCHandler::exec(MCExecContext& ctxt, MCParameter *plist)
 			}
 			delete vars[nvnames];
 		}
-		delete vars;
+		delete[] vars; /* Allocated with new[] */
 	}
 	params = oldparams;
 	nparams = oldnparams;
diff --git a/engine/src/hndlrlst.cpp b/engine/src/hndlrlst.cpp
@@ -162,11 +162,11 @@ void MCHandlerlist::reset(void)
 
 	for(uint32_t i = 0; i < nvars; i++)
 		MCValueRelease(vinits[i]);
-	delete vinits;
+	delete[] vinits; /* Allocated with new[] */
 	vinits = NULL;
 	nvars = 0;
 
-	delete globals;
+	delete[] globals; /* Allocated with new[] */
 	globals = NULL;
 	nglobals = 0;
 
@@ -175,7 +175,7 @@ void MCHandlerlist::reset(void)
 		MCNameDelete(cinfo[i] . name);
 		MCValueRelease(cinfo[i] . value);
 	}
-	delete cinfo;
+	delete[] cinfo; /* Allocated with new[] */
 	cinfo = NULL;
 	nconstants = 0;
 }
diff --git a/engine/src/image.cpp b/engine/src/image.cpp
@@ -1032,8 +1032,10 @@ IO_stat MCImage::extendedload(MCObjectInputStream& p_stream, uint32_t p_version,
 
             if (t_stat == IO_NORMAL)
 			{
-				/* UNCHECKED */ MCMemoryNewArray(s_control_color_count, s_control_colors);
-				/* UNCHECKED */ MCMemoryNewArray(s_control_color_count, s_control_color_names);
+				s_control_colors = new MCColor[s_control_color_count];
+				s_control_color_names = new MCStringRef[s_control_color_count];
+				if (nil == s_control_colors || nil == s_control_color_names)
+					t_stat = checkloadstat(IO_ERROR);
 			}
 
 			for (uint32_t i = 0; t_stat == IO_NORMAL && i < s_control_color_count; i++)
@@ -1079,6 +1081,12 @@ IO_stat MCImage::extendedload(MCObjectInputStream& p_stream, uint32_t p_version,
 	if (t_stat == IO_NORMAL)
 		t_stat = MCObject::extendedload(p_stream, p_version, p_remaining);
 
+	if (t_stat != IO_NORMAL)
+	{
+		delete[] s_control_colors;
+		delete[] s_control_color_names;
+	}
+
 	return t_stat;
 }
 
@@ -1426,10 +1434,10 @@ IO_stat MCImage::load(IO_handle stream, uint32_t version)
 	// MW-2013-09-05: [[ Bug 11127 ]] At this point the color/pixmap fields in the object
 	//   will pertain to the image colors. This isn't what we want anymore, so free them
 	//   (an RLE compressed rep will already have extracted the info it needs).
-	MCMemoryDeleteArray(colors);
+	delete[] colors; /* Allocated with new[] */
 	for (uint32_t i = 0; i < ncolors; i++)
 		MCValueRelease(colornames[i]);
-	MCMemoryDeleteArray(colornames);
+	delete[] colornames; /* Allocated with new[] */
 	MCMemoryDeleteArray(patterns);
 	ncolors = 0;
 	npatterns = 0;
diff --git a/engine/src/keywords.cpp b/engine/src/keywords.cpp
@@ -982,9 +982,9 @@ MCSwitch::~MCSwitch()
 	delete cond;
 	while (ncases--)
 		delete cases[ncases];
-	delete cases;
+	delete[] cases; /* Allocated with new[] */
 	deletestatements(statements);
-	delete caseoffsets;
+	delete[] caseoffsets; /* Allocated with new[] */
 }
 
 Parse_stat MCSwitch::parse(MCScriptPoint &sp)
diff --git a/engine/src/lnxpsprinter.cpp b/engine/src/lnxpsprinter.cpp
@@ -100,9 +100,10 @@ void MCPSPrinter::DoInitialize(void)
 void MCPSPrinter::DoFinalize(void)
 {
     delete m_pdf_printer;
-    
-	if ( m_printersettings . printername != NULL ) 
-		delete m_printersettings . printername ;
+
+    /* Allocated by MCStringConvertToCString() */
+	if ( m_printersettings . printername != NULL )
+		MCMemoryDeleteArray(m_printersettings . printername);
 
 	if ( m_printersettings . outputfilename != NULL ) 
 		delete (m_printersettings . outputfilename -7);	// Need to subtract 7 here as we added 7 to skip the "file://" part
diff --git a/engine/src/object.cpp b/engine/src/object.cpp
@@ -304,13 +304,13 @@ MCObject::~MCObject()
 	MCundos->freeobject(this);
 	delete hlist;
 	MCNameDelete(_name);
-	delete colors;
+	delete[] colors; /* Allocated with new[] */
 	if (colornames != nil)
 	{
 		while (ncolors--)
 			if (colornames[ncolors] != nil)
 				MCValueRelease(colornames[ncolors]);
-		delete colornames;
+		delete[] colornames; /* Allocated with new[] */
 	}
 
 	MCValueRelease(_script);
@@ -1784,8 +1784,8 @@ uint2 MCObject::createcindex(uint2 di)
 	}
 	if (oldcolors != NULL)
 	{
-		delete oldcolors;
-		delete oldnames;
+		delete[] oldcolors; /* Allocated with new[] */
+		delete[] oldnames; /* Allocated with new[] */
 	}
 	return ri;
 }
@@ -4846,7 +4846,7 @@ void MCObject::clearfontattrs(void)
 		return;
 
 	MCNameDelete(m_font_attrs -> name);
-	delete m_font_attrs;
+	MCMemoryDelete(m_font_attrs); /* Allocated with MCMemoryNew() */
 	m_font_attrs = nil;
 
 	// MW-2012-02-19: [[ SplitTextAttrs ]] Unset all the individual fontattr flags.
diff --git a/engine/src/objectstream.cpp b/engine/src/objectstream.cpp
@@ -40,7 +40,7 @@ MCObjectInputStream::MCObjectInputStream(IO_handle p_stream, uint32_t p_remainin
 
 MCObjectInputStream::~MCObjectInputStream(void)
 {
-	delete (char *)m_buffer;
+	delete[] (char *)m_buffer; /* Allocated with new[] */
 }
 
 // Flushing reads and discards the rest of the stream
@@ -265,14 +265,14 @@ IO_stat MCObjectInputStream::ReadStringRefNew(MCStringRef &r_value, bool p_suppo
 	if (ReadU32(t_length) != IO_NORMAL)
 		return IO_ERROR;
 	
-	MCAutoPointer<char> t_bytes;
-	if (!MCMemoryNewArray(t_length, &t_bytes))
+	MCAutoArray<char> t_bytes;
+	if (!t_bytes.New(t_length))
 		return IO_ERROR;
 	
-	if (Read(*t_bytes, t_length) != IO_NORMAL)
+	if (Read(t_bytes.Ptr(), t_length) != IO_NORMAL)
 		return IO_ERROR;
 	
-	if (!MCStringCreateWithBytes((const uint8_t *)*t_bytes, t_length, kMCStringEncodingUTF8, false, r_value))
+	if (!MCStringCreateWithBytes((const uint8_t *)t_bytes.Ptr(), t_length, kMCStringEncodingUTF8, false, r_value))
 		return IO_ERROR;
 	
 	return IO_NORMAL;
@@ -430,7 +430,7 @@ MCObjectOutputStream::MCObjectOutputStream(IO_handle p_stream)
 
 MCObjectOutputStream::~MCObjectOutputStream(void)
 {
-	delete (char *)m_buffer;
+	delete[] (char *)m_buffer; /* Allocated with new[] */
 }
 
 IO_stat MCObjectOutputStream::WriteTag(uint32_t p_flags, uint32_t p_length)
diff --git a/engine/src/paragraf.cpp b/engine/src/paragraf.cpp
@@ -477,7 +477,7 @@ IO_stat MCParagraph::load(IO_handle stream, uint32_t version, bool is_ext)
 	if (version < 7000)
 	{
 		uint32_t t_length;
-		MCAutoPointer<char> t_text_data;
+		MCAutoCustomPointer<char,MCMemoryDeleteArray> t_text_data;
         
 		// This string can contain a mixture of Unicode and native - t_length is the number
         // of bytes.
diff --git a/engine/src/parentscript.cpp b/engine/src/parentscript.cpp
@@ -154,7 +154,7 @@ void MCParentScriptUse::ClearVars(void)
 		delete m_locals[i];
 
 	// Finally delete the locals array
-	delete m_locals;
+	delete[] m_locals; /* Allocated with new[] */
 	
 	m_locals = NULL;
 	m_local_count = 0;
diff --git a/engine/src/stack.cpp b/engine/src/stack.cpp
diff --git a/engine/src/stacklst.cpp b/engine/src/stacklst.cpp
diff --git a/engine/src/uidc.cpp b/engine/src/uidc.cpp
diff --git a/revxml/src/revxml.cpp b/revxml/src/revxml.cpp

Original file line number	Diff line number	Diff line change
`@@ -154,7 +154,7 @@ MCAudioClip::~MCAudioClip()`
`154`	`154`	`stop(True);`
`155`	`155`	`MCacptr = NULL;`
`156`	`156`	`}`
`157`		`- delete samples;`
	`157`	`+ delete[] samples; /* Allocated with new[] */`
`158`	`158`	`delete osamples;`
`159`	`159`	`#ifdef TARGET_PLATFORM_LINUX`
`160`	`160`	`if ( x11audio != NULL )`
Original file line number	Diff line number	Diff line change
`@@ -178,8 +178,8 @@ MCDispatch::~MCDispatch()`
`178`	`178`	`}`
`179`	`179`	`delete fonts;`
`180`	`180`
`181`		`- delete startdir;`
`182`		`- delete enginedir;`
	`181`	`+ MCMemoryDeleteArray(startdir); /* Allocated by MCStringConvertToCString() */`
	`182`	`+ MCMemoryDeleteArray(enginedir); /* Allocated by MCStringConvertToCString() */`
`183`	`183`
`184`	`184`	`delete m_externals;`
`185`	`185`	`// AL-2015-02-10: [[ Standalone Inclusions ]] Delete library mapping`
Original file line number	Diff line number	Diff line change
`@@ -1395,7 +1395,7 @@ class MCLinuxDesktop: public MCSystemInterface`
`1395`	`1395`	`}`
`1396`	`1396`	`}`
`1397`	`1397`
`1398`		`- delete t_entry_path;`
	`1398`	`+ delete[] t_entry_path; /* Allocated with new[] */`
`1399`	`1399`	`closedir(dirptr);`
`1400`	`1400`
`1401`	`1401`	`return t_success;`
Original file line number	Diff line number	Diff line change
`@@ -98,8 +98,8 @@ MCEPS::MCEPS(const MCEPS &sref) : MCControl(sref)`
`98`	`98`
`99`	`99`	`MCEPS::~MCEPS()`
`100`	`100`	`{`
`101`		`- delete postscript;`
`102`		`- delete prolog;`
	`101`	`+ delete[] postscript; /* Allocated with new[] */`
	`102`	`+ delete[] prolog; /* Allocated with new [] */`
`103`	`103`	`delete pageIndex;`
`104`	`104`	`delete image;`
`105`	`105`	`}`
Original file line number	Diff line number	Diff line change
`@@ -961,7 +961,7 @@ bool MCArraysCopyExtents(MCArrayRef self, array_extent_t*& r_extents, uindex_t&`
`961`	`961`
`962`	`962`	`while (MCArrayIterate(self, t_index, t_key, t_value))`
`963`	`963`	`{`
`964`		`- MCAutoPointer<index_t> t_indexes;`
	`964`	`+ MCAutoCustomPointer<index_t,MCMemoryDeleteArray> t_indexes;`
`965`	`965`	`uindex_t t_index_count;`
`966`	`966`	`bool t_all_integers;`
`967`	`967`	`if (!MCArraysSplitIndexes(t_key, &t_indexes, t_index_count, t_all_integers))`
Original file line number	Diff line number	Diff line change
`@@ -1485,7 +1485,7 @@ void MCStack::SetStackFiles(MCExecContext& ctxt, MCStringRef p_files)`
`1485`	`1485`	`MCValueRelease(stackfiles[nstackfiles].stackname);`
`1486`	`1486`	`MCValueRelease(stackfiles[nstackfiles].filename);`
`1487`	`1487`	`}`
`1488`		`- delete stackfiles;`
	`1488`	`+ delete[] stackfiles; /* Allocated with new[] */`
`1489`	`1489`
`1490`	`1490`	`if (stringtostackfiles(p_files, &stackfiles, nstackfiles))`
`1491`	`1491`	`return;`
Original file line number	Diff line number	Diff line change
`@@ -353,9 +353,9 @@ Exec_stat MCExternalV0::Handle(MCObject *p_context, Handler_type p_type, uint32_`
`353`	`353`	`if (args != NULL)`
`354`	`354`	`{`
`355`	`355`	`while (nargs--)`
`356`		`- delete args[nargs];`
	`356`	`+ MCMemoryDeleteArray(args[nargs]); /* Allocated with MCStringNormalizeAndConvertToCString */`
`357`	`357`
`358`		`- delete args;`
	`358`	`+ delete[] args; /* Allocated with new[] */`
`359`	`359`	`}`
`360`	`360`
`361`	`361`	`if (Xerr)`