|
Memory Leak in the Mailer
The mailer provided in iDirt 1.82 has a number of problems. Quite a few of the menu options do not perform bounds checking; certain of the options only allow a single digit 0-9; the mailer does not prevent you from mailing mobiles; etc. I've run across a new one (to me, at least) that creates a memory leak and allows for a SIGSEGV condition. If you've already seen this error and fixed it, my apologies for thinking it's new. I hadn't seen it before and I'm sure there are some people out there who still haven't. In mailer.c in the function quit_mailer(), stock iDirt has the following code:
cur_player->inmailer = False;
FREE (cur_player->Mailer.msgidx);
bprintf ("&+WExiting &+Ci&+WDiRT Mailer\n");
strcpy (cur_player->prompt, cur_player->Mailer.old_prompt);
strcpy (cur_player->cprompt, build_prompt (mynum));
bprintf ("%s", cur_player->cprompt);
replace_input_handler (cur_player->Mailer.old_handler);
return;
The problem is in the FREE call. After freeing the memory pointed to by msgidx, msgidx should be set to NULL to prevent the attempt to free memory a second time should the mailer be entered again. If you have no messages to load and you enter the mailer, msgidx will not be reset. This piece of code works much better for me. Replace the call to FREE with:
if (cur_player->Mailer.msgidx != NULL) {
FREE (cur_player->Mailer.msgidx);
cur_player->Mailer.msgidx = NULL;
}
The if-statement is just a safety-check, attempting to free a null pointer seems fatal on some systems. The assignment of NULL to msgidx is absolutely necessary, however, since the memory that was once pointed to could very well be in use by something else now. Unfinished Functionality in the Mailer While researching a memory leak in iDirt 1.82's mailer, I ran across a piece of functionality that wasn't completed. In mailer.c, reindex_mail reads as:
reindex_mail (int plr)
{
MSGIDX *msgidxtmp;
int me = real_mynum, i;
setup_globals (plr);
msgidxtmp = NEW (MSGIDX, cur_player->Mailer.lastmsg);
for (i = 0; i < cur_player->Mailer.lastmsg; i++) {
msgidxtmp[i].offset = msg_idx_offset (mynum, i);
msgidxtmp[i].delete = msg_idx_delete (mynum, i);
}
FREE (cur_player->Mailer.msgidx);
fclose (cur_player->Mailer.mailbox);
open_mailbox (pname (mynum), READ);
rewind (cur_player->Mailer.mailbox);
cur_player->Mailer.lastmsg = 0;
while (!feof (cur_player->Mailer.mailbox)) {
if (fgetc (cur_player->Mailer.mailbox) == EOM_MARKER)
++cur_player->Mailer.lastmsg;
}
rewind (cur_player->Mailer.mailbox);
read_msgidx ();
setup_globals (me);
}
The first thing that occurred to me here is that a temporary mail index is created and then never released. This causes a memory leak that can cause a crash over time. The second thing I noticed is that the mail index is being saved before being reloaded; this was done, presumably, to keep the current "delete" status of any messages. I tested this by entering the mailer, deleting some messages, then forwarding some mail to myself (new mail received while in the mailer causes this function to be called). Sure enough, all of my deleted messages were no longer marked for deletion. I recommend the following replacement code. It cleans up the memory leak and makes sure to use the saved information.
reindex_mail (int plr)
{
MSGIDX *msgidxtmp;
int me = real_mynum, i, j;
setup_globals (plr);
msgidxtmp = NEW (MSGIDX, cur_player->Mailer.lastmsg);
for (i = 0; i < cur_player->Mailer.lastmsg; i++) {
msgidxtmp[i].offset = msg_idx_offset (mynum, i);
msgidxtmp[i].delete = msg_idx_delete (mynum, i);
}
j = cur_player->Mailer.lastmsg;
FREE (cur_player->Mailer.msgidx);
fclose (cur_player->Mailer.mailbox);
open_mailbox (pname (mynum), READ);
rewind (cur_player->Mailer.mailbox);
cur_player->Mailer.lastmsg = 0;
while (!feof (cur_player->Mailer.mailbox)) {
if (fgetc (cur_player->Mailer.mailbox) == EOM_MARKER)
++cur_player->Mailer.lastmsg;
}
rewind (cur_player->Mailer.mailbox);
read_msgidx ();
setup_globals (me);
for ( i = 0; i < j; i++ ) {
msg_idx_offset(mynum, i) = msgidxtmp[i].offset;
msg_idx_delete(mynum, i) = msgidxtmp[i].delete;
}
FREE(msgidxtmp);
}
|