Leaking Memory in a sendmail milter I wrote?

I am hoping this is the right place to get some help with this. I have written a sendmail milter for the purpose of allowing pop users to securely submit spam/ham to our spam filter for learning. Because the user's use pop accounts it's easiest for me to have them forward their incorrectly marked messages to a specific email address and only submit messages sent by an authenticated user to the spam learning system. I have written a Milter or Sendmail Filter to that end.

Everything seems to work as expected except over time it seems to eat up memory and I am not exactly sure why. I got some help from someone on the comp.mail.sendmail group which helped a little but it is still leaking memory somewhere.

This is my first project in C of any value. Previously I have only written small tutorial like programs from books or internet tutorials. I think I have a basic grasp of the concepts but I am still shaky in many areas.

The sendmail milter API calls a set of user supplied functions for each part of the smtp transaction, in order to keep track of message and connection specific data the API passes a pointer to a private data structure that is set up to hold all the information required for the milter. (If this is a review for one of you experts or described incorrectly please forgive me.).

I believe my problem has to be in the cleanup functions that receive the pointer to this data when a message ends and then clears out the message specific data and then when the smtp transaction ends and should then free all the memory used to store message/connection information again for use by the OS.

Here is a small snippet of the functions I am using. If you would like to see the full source code you can download a zipped tarball from http://www.fletchcom.com/spamhammilt-0.1.tar.gz

Code:

void cleanup_envelope ( struct mlfi_priv *priv )
{
	if ( priv->envfrom )
	{
		memset ( priv->envfrom,'\0',sizeof ( priv->envfrom ) );
	}
	if ( priv->envrcpt )
	{
		memset ( priv->envrcpt,'\0',sizeof ( priv->envrcpt ) );
	}
	if ( priv->fd > -1 )
	{
		close ( priv->fd );
		priv->fd = '\0';
	}
	if ( strlen ( priv->filename ) >0 )
	{
		memset ( priv->filename,'\0',sizeof ( priv->filename ) );
	}
}

void cleanup_connection ( struct mlfi_priv *priv )
{
	if ( priv->rhost )
	{
		memset ( priv->rhost,'\0',sizeof ( priv->rhost ) );
	}
	if ( priv->raddr )
	{
		memset ( priv->raddr,'\0',sizeof ( priv->raddr ) );
	}
	if ( priv->loglev > '\0' )
	{
		priv->loglev = '\0';
	}
	if ( priv->log_email > '\0' )
	{
		priv->log_email = '\0';
	}
	if ( priv->auth > '\0' )
	{
		priv->auth = '\0';
	}
	if ( priv->authid )
	{
		memset ( priv->authid,'\0',sizeof ( &priv->authid ) );
	}
}

int cleanup ( SMFICTX *ctx,int level )
{
	struct mlfi_priv *priv;
	priv = ( struct mlfi_priv * ) smfi_getpriv ( ctx );
	if ( priv == NULL )
	{
		return TRUE;
	}
	switch ( level )
	{
		case ENVELOPE:
			( void ) cleanup_envelope ( priv );
			break;
		case CONNECTION:
			( void ) cleanup_envelope ( priv );
			( void ) cleanup_connection ( priv );
			free ( priv );
			break;
	}
	smfi_setpriv ( ctx,NULL );
	return TRUE;
}

Not sure what a "milter" is, but I am sure the code you posted does not get or free any memory. You are closing a file descriptor, but I don't think there is any responsibility on your shoulders for freeing any residual memory with that.

(EDIT - oops - just saw the free. Still no help).

Are you zeroing out any of your pointers that might have had a pointer to a malloc()'ed area?

Dino,

The memory that is supposed to be freed by these functions is being malloc()'ed by each user supplied callback function. I'll post one of the functions here to show you what they each do.

This is the first function called by sendmail for each message when a connection is made to the server.

Thanks for all the help. If it makes much difference the milters run multithreaded. The API takes care of all the multi threading which is why I only get a pointer to my private data for each function. I will also include the private data structure I have defined and hopefully that will give enough info to help you see where I might be going wrong.

Code:

sfsistat mlfi_connect ( SMFICTX *ctx,char *hname, _SOCK_ADDR *haddr )
{
        /*
        Set up and allocate space for a private structure to store all the email data in.
        */
        smfi_setdbg ( 6 );
        struct mlfi_priv *priv;
        priv = ( struct mlfi_priv * ) smfi_getpriv ( ctx );
        if ( priv == NULL )
        {
                priv = ( struct mlfi_priv * ) malloc ( sizeof ( struct mlfi_priv ) );
                if ( priv == NULL )
                {
                        syslog ( LOG_ERR,"mlfi_connect: Error getting private data storage for the connection" );
                        return SMFIS_TEMPFAIL;
                }
        }

        /*
        Set every byte of the structure to null, this cleans up the memory from previous data stored there and prevents
        garbage data from entering the program
        */
        memset ( priv,'\0',sizeof ( struct mlfi_priv ) );
        /*
        Register the pointer to the structure with sendmail so that it can pass it back to us in later callbacks
        */
        if ( smfi_setpriv ( ctx,priv ) == MI_FAILURE )
        {
                free ( priv );
                return SMFIS_TEMPFAIL;
        }
        /*
        Store the remote ip if applicable and hostname. Default to "Local" if not available.  E.g. Sendmail was called from the command line
        */
        const void *mysaddr = NULL;
        char host[INET6_ADDRSTRLEN];
        switch ( haddr->sa_family )
        {
                default:
                        syslog ( LOG_ERR,"mlfi_connect: unsupported sa_family" );
                        break;
                case AF_INET:
                        mysaddr = ( const void * ) & ( ( struct sockaddr_in * ) haddr )->sin_addr.s_addr;
                        break;
                case AF_INET6:
                        mysaddr = ( const void * ) & ( ( struct sockaddr_in6 * ) haddr )->sin6_addr;
                        break;
        }
        if ( !inet_ntop ( haddr->sa_family,mysaddr,host,sizeof ( host ) ) )
        {
                syslog ( LOG_ERR,"mlfi_connect: inet_ntop failed" );
                strcpy ( host,"*" );
        }
        strncpy ( priv->rhost,hname,strlen ( hname ) );
        priv->rhost[strlen ( hname ) ]= '\0';
        strncpy ( priv->raddr,host,sizeof ( host ) );
        priv->rhost[sizeof ( host ) ] = '\0';
        priv->log_email = FALSE;
        return SMFIS_CONTINUE;
}

Private data structure

Code:

struct mlfi_priv
{
        char envfrom[ADDRLEN+1];
        char envrcpt[ADDRLEN+1];
        char rhost[ADDRLEN+1];
        char raddr[ADDRLEN+1];
        char filename[ADDRLEN+1];
        char authid[ADDRLEN+1];
        int auth;
        int fd;
        int loglev;
        int log_email;
};

ADDRLEN is 324

Okay. So you malloc a priv, you do smfi_setpriv on it, whatever that is. Can you use this structure after this function ends? (I.e., does smfi_setpriv keep the pointer priv?) When should priv "go away"?

Yes smfi_setpriv keeps the pointer for passing on to the next function in the chain, checking of headers, message body, etc.
The priv data should be free'd at the end of all processing which is what the original cleanup functions were for.
The function cleanup_envelope is supposed to reset all message related functions to zero in case there is more than one message coming in on the connection. This way the variables can safely be used again to store message related data between calls to the email.
The cleanup_connection is called when the connection to the server ends and I therefore need to free up all the memory associated with the connection which is when priv should be free'd.

I hope that helps clarify things.

It looks like every time you clean up ENVELOPE instead of CONNECTION, bad things happen; priv gets disassociated from ctx (you call the setpriv with NULL), but the memory isn't freed. (Unless you have the same pointer in multiple places.)

Are you sure that doing memset(pointer, 0, sizeof(pointer)); is the right thing to do? What is priv->enfrom, for example? If it's a pointer, you're only zeroing out 4 or 8 bytes, depending on whether you're running 32-bit or 64-bit.

Originally Posted by tabstop

It looks like every time you clean up ENVELOPE instead of CONNECTION, bad things happen; priv gets disassociated from ctx (you call the setpriv with NULL), but the memory isn't freed. (Unless you have the same pointer in multiple places.)

tabstop,

Thank you for pointing that out. I think that has cleared up a big portion of the problems. I'll keep working and if anyone else has any suggestions please let me know.

Originally Posted by IceDane

Are you sure that doing memset(pointer, 0, sizeof(pointer)); is the right thing to do? What is priv->enfrom, for example? If it's a pointer, you're only zeroing out 4 or 8 bytes, depending on whether you're running 32-bit or 64-bit.

IceDane,

I am not sure if memset is the right thing to do or not. What I am trying to do is cleanup the space being used by priv->envfrom for example, because in this same transaction there might be another message that comes in and it should have clean variables to store it's information in.

If you can suggest a better way I would appreciate it. I really am just trying to make an educated guess on some of these things since I still don't know c very well.

Thank you