Mem-leak in Client::QuestTroupe()

[Deawin]

Around line 2120 in client.cpp:

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:


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

[Deawin]

Maybe remove result at all since nobody cares for the result set here anyway.

[Deawin]

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

[kathgar]

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

[Deawin]

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"


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

[Trumpcard]

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 ?

[Deawin]

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

[Trumpcard]

I didnt notice that attatchments had been disabled... Sorry bout that..

I'll send you a PM...

[DeletedUser]

Around line 2120 in client.cpp:

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:


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:
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.
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:
if (database.RunQuery(query, MakeAnyLenString(...), ...) {
...
safe_delete_array(query);
} else {
safe_delete_array(query);
}

so you dont memleak the query variable. =)