Go Back   EQEmulator Home > EQEmulator Forums > Archives > Archive::Development > Archive::Development
Reload this Page Mem-leak in Client::QuestTroupe()

Archive::Development Archive area for Development's posts that were moved here after an inactivity period of 90 days.

Reply
 
Thread Tools Display Modes
  #1  
Old 08-02-2003, 05:06 AM
Deawin
Fire Beetle
  
Join Date: Jun 2003
Location: Vienna, Austria
Posts: 11
Default Mem-leak in Client::QuestTroupe()
Around line 2120 in client.cpp:
Code:
Message(13,"Attempting to delete petition number: %i",atoi(sep->argplus[1]));
			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf, &result)) {
				safe_delete_array(query);
				LogFile->write(EQEMuLog::Normal,"Delete petition request from %s, petition number:", GetName(), atoi(sep->argplus[1]) );
			}
			//mysql_free_result(result); // If uncommented crashes zone. :/
If RunQuery() succeedes, result is never freed. I just moved the commented mysql_free_result below the if {} block into it.
Here is the corrected code piece:

Code:
Message(13,"Attempting to delete petition number: %i",atoi(sep->argplus[1]));
			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf, &result)) {
				safe_delete_array(query);
				mysql_free_result(result);
				LogFile->write(EQEMuLog::Normal,"Delete petition request from %s, petition number:", GetName(), atoi(sep->argplus[1]) );
			}
Regards,
Markus
Reply With Quote
  #2  
Old 08-02-2003, 05:09 AM
Deawin
Fire Beetle
  
Join Date: Jun 2003
Location: Vienna, Austria
Posts: 11
Default
Maybe remove result at all since nobody cares for the result set here anyway.
Reply With Quote
  #3  
Old 08-02-2003, 05:29 AM
Deawin
Fire Beetle
  
Join Date: Jun 2003
Location: Vienna, Austria
Posts: 11
Default
Okay, correct me if I am wrong but if RunQuery() returns true, mysql_free_result() must be called on the result pointer. Otherwise, if RunQuery() returns false, mysql_fee_result() must not be freed.
If thats true, here are some locations where result is not, or uncorrectly freed.
If thats wrong, ignore all of the bugs below.

All line numbers from latest CVS source.


client.cpp - Client::NormalGM() around line 4104 in handler for "#spawnfix":
mysql_free_result() is called when RunQuery() returns false. Possible GPF.
And at line 4095 there is a problem with mysql_free_result() if the second RunQuery call fails. In this case, result of the first RunQuery() is freed twice.

client.cpp - Client::NormalUser() around line 4359 in handler for "#viewmessage":
result pointer used in two RunQuery() calls without freeing after first use. Memory leak. Looks like result of second RunQuery() isn't used anyway so maybe remove it.

client.cpp - Database::CheckGuildDoor() around line 6981:
if {} else {} block with return statements. code below won't ever be reached. might want to ignore this... wont be a problem anyway.

client.cpp - Client::VHServerOp() around line 1590:
result pointer is never freed. Memory leak.

client.cpp - Client::LeadGM() around line 3215:
result poitner is never freed. Memory leak.

Regards,
Markus
Reply With Quote
  #4  
Old 08-02-2003, 07:54 AM
kathgar
Discordant
  
Join Date: May 2002
Posts: 434
Default
Yeah.. freeing if RunQuery returned FAIL..is quite bad...I'll check all of those you mentioned out or atleast forward it to someone else
__________________
++[>++++++<-]>[<++++++>-]<.>++++[>+++++<-]>[<
+++++>-]<+.+++++++..+++.>>+++++[<++++++>-]<+
+.<<+++++++++++++++.>.+++.------.--------.>+.
Reply With Quote
  #5  
Old 08-04-2003, 08:24 PM
Deawin
Fire Beetle
  
Join Date: Jun 2003
Location: Vienna, Austria
Posts: 11
Default
If you didn't have time to check this yet, I have made a patch against the latest CVS source that fixes all this stuff in client.cpp

Apply with "patch client.cpp <patchfile"

Code:
--- client.cpp	2003-08-05 10:18:30.000000000 +0200
+++ working/client.cpp	2003-08-05 09:59:36.000000000 +0200
@@ -1596,9 +1596,8 @@
 		else {
 			char errbuf[MYSQL_ERRMSG_SIZE];
 			char *query = 0;
-			MYSQL_RES *result;
 			Message(15,"Setting NPC %u to faction %i",target->CastToNPC()->GetNPCTypeID(),atoi(sep->argplus[1]));
-			if (database.RunQuery(query, MakeAnyLenString(&query, "update npc_types set npc_faction_id=%i where id=%i",atoi(sep->argplus[1]),target->CastToNPC()->GetNPCTypeID()), errbuf, &result)) {
+			if (database.RunQuery(query, MakeAnyLenString(&query, "update npc_types set npc_faction_id=%i where id=%i",atoi(sep->argplus[1]),target->CastToNPC()->GetNPCTypeID()), errbuf)) {
 				delete[] query;
 			}
 		}
@@ -2124,7 +2123,6 @@
 			char errbuf[MYSQL_ERRMSG_SIZE];
 			char *query = 0;
 			//int queryfound = 0;
-			MYSQL_RES *result;
 			//MYSQL_ROW row;
 			//Petition* newpet;
 			//char* blah32;
@@ -2132,7 +2130,7 @@
 			//char* querystring;
 			//querystring=strcat("DELETE from petitions where petid=",blah32);
 			Message(13,"Attempting to delete petition number: %i",atoi(sep->argplus[1]));
-			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf, &result)) {
+			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf)) {
 				safe_delete_array(query);
 				LogFile->write(EQEMuLog::Normal,"Delete petition request from %s, petition number:", GetName(), atoi(sep->argplus[1]) );
 			}
@@ -3230,6 +3228,7 @@
 						x++;
 					}
 					Message(0, " %u backups found.", x);
+					mysql_free_result(result);
 				}
 				else {
 					Message(13, "Query error: '%s' %s", query, errbuf);
@@ -4091,7 +4090,6 @@
 			if (database.RunQuery(query, MakeAnyLenString(&query, "SELECT id from spawn2 where zone='%s' AND x='%f' AND y='%f' AND heading='%f'", zone->GetShortName(), target->GetX(), target->GetY(), target->GetHeading()), errbuf, &result)) {
 				if (mysql_num_rows(result) == 1) {
 					row = mysql_fetch_row(result);
-					mysql_free_result(result);
 					tmp = atoi(row[0]);
 					if(database.RunQuery(query, MakeAnyLenString(&query, "UPDATE spawn2 SET x='%f', y='%f', z='%f', heading='%f' WHERE id='%i'",this->GetX(), this->GetY(), this->GetZ(), this->GetHeading(),tmp), errbuf, 0)) {
 						Message(0, "Updating coordinates successful.");
@@ -4099,16 +4097,14 @@
 					}
 					else {
 						Message(0, "Update failed, UPDATE command error!");
-						mysql_free_result(result);
 					}
 				}
 				else {
-					mysql_free_result(result);
 					Message(0, "Update failed, duplicate spawns detected!");
 				}
+				mysql_free_result(result);
 			}
 			else {
-				mysql_free_result(result);
 				Message(0, "Update failed, SELECT command error!");
 			}

@@ -4361,16 +4357,16 @@
 				//char *name=this->name;
 				if (mysql_num_rows(result)==1){
 				row = mysql_fetch_row(result);
+				mysql_free_result(result);
 				if (strcasecmp((char*) this->name, (char*) row[2]) == 0){
 					Message(15,"ID: %s,Sent Date: %s,Sender: %s,Message: %s",row[0],row[1],row[3],row[4]);
-					database.RunQuery(query, MakeAnyLenString(&query, "Delete from tellque where id=%s",row[0]), errbuf, &result);
+					database.RunQuery(query, MakeAnyLenString(&query, "Delete from tellque where id=%s",row[0]), errbuf);
 				}
 				else
 					Message(13,"Invalid Message Number, check the number and try again.");
 				}
 				else
 					Message(13,"Invalid Message Number, check the number and try again.");
-				mysql_free_result(result);
 			}
 			safe_delete_array(query);

@@ -7029,7 +7025,8 @@
 				mysql_free_result(result);
 				return false;
 			}
-
+
+			// code below will never be reached
 			mysql_free_result(result);
 			return false;
 		}
Regards,
Markus
Reply With Quote
  #6  
Old 08-05-2003, 12:23 AM
Trumpcard
Demi-God
  
Join Date: Jan 2002
Location: Charlotte, NC
Posts: 2,614
Default
I hate to ask this, but can you attatch it in text form. I can't paste properly off the forums, and I don't feel like reformatting a large patch file..


Thanks! Also, CVS updated last night (I had to fix some of the NewSource jobs) , so can you check the patch against last night CVS build ?
__________________
Quitters never win, and winners never quit, but those who never win and never quit are idiots.
Reply With Quote
  #7  
Old 08-05-2003, 02:18 AM
Deawin
Fire Beetle
  
Join Date: Jun 2003
Location: Vienna, Austria
Posts: 11
Default
This patch is against the latest CVS already.

And I hate to ask that too, but how can I attach a file to my posts?
Anyway, if it's easier for you I can send you an email as well or put this stuff on some ftp server.

Regards,
Markus
Reply With Quote
  #8  
Old 08-05-2003, 02:50 AM
Trumpcard
Demi-God
  
Join Date: Jan 2002
Location: Charlotte, NC
Posts: 2,614
Default
I didnt notice that attatchments had been disabled... Sorry bout that..

I'll send you a PM...
__________________
Quitters never win, and winners never quit, but those who never win and never quit are idiots.
Reply With Quote
  #9  
Old 08-23-2003, 05:57 AM
DeletedUser
Fire Beetle
  
Join Date: Sep 2002
Posts: 0
Default Re: Mem-leak in Client::QuestTroupe()
Quote:
Originally Posted by Deawin
Around line 2120 in client.cpp:
Code:
Message(13,"Attempting to delete petition number: %i",atoi(sep->argplus[1]));
			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf, &result)) {
				safe_delete_array(query);
				LogFile->write(EQEMuLog::Normal,"Delete petition request from %s, petition number:", GetName(), atoi(sep->argplus[1]) );
			}
			//mysql_free_result(result); // If uncommented crashes zone. :/
If RunQuery() succeedes, result is never freed. I just moved the commented mysql_free_result below the if {} block into it.
Here is the corrected code piece:

Code:
Message(13,"Attempting to delete petition number: %i",atoi(sep->argplus[1]));
			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf, &result)) {
				safe_delete_array(query);
				mysql_free_result(result);
				LogFile->write(EQEMuLog::Normal,"Delete petition request from %s, petition number:", GetName(), atoi(sep->argplus[1]) );
			}
Hmm, if i remember how stuff works right, it'd be better to change it to:
Code:
			if (database.RunQuery(query, MakeAnyLenString(&query, "DELETE from petitions where petid=%i",atoi(sep->argplus[1])), errbuf)) {
and drop the result altogether, since i dont think DELETE calls return a result set at all - and RunQuery should detect this as an error condition (result set requested on query that doesnt return one) and return false, and set the errbuf accordingly.
Code:
strcpy(errbuf, "DBcore::RunQuery: No Result");
I think affected_rows gets set on delete queries though, i might be wrong there however, but if it does that'd be the response from the mysql server you'd want to look at to find out if the query worked.


Another note, it'd be good to have:
Code:
			if (database.RunQuery(query, MakeAnyLenString(...), ...) {
				...
				safe_delete_array(query);
			} else {
				safe_delete_array(query);
			}
so you dont memleak the query variable. =)
Reply With Quote
Reply

« Previous Thread | Next Thread »

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts
BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Forum Jump

   

All times are GMT -4. The time now is 07:48 AM.

EQEmulator - Archive - Top

 

Everquest is a registered trademark of Daybreak Game Company LLC.
EQEmulator is not associated or affiliated in any way with Daybreak Game Company LLC.
Except where otherwise noted, this site is licensed under a Creative Commons License.
       
Powered by vBulletin®, Copyright ©2000 - 2025, Jelsoft Enterprises Ltd.
Template by Bluepearl Design and vBulletin Templates - Ver3.3