Leaking Memory in a sendmail milter I wrote?

This is a discussion on Leaking Memory in a sendmail milter I wrote? within the C Programming forums, part of the General Programming Boards category; I am hoping this is the right place to get some help with this. I have written a sendmail milter ...

  1. #1
    Registered User
    Join Date
    Oct 2008
    Posts
    17

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

  2. #2
    Jack of many languages Dino's Avatar
    Join Date
    Nov 2007
    Location
    Katy, Texas
    Posts
    2,309
    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?
    Mac and Windows cross platform programmer. Ruby lover.

    Quote of the Day
    12/20: Mario F.:I never was, am not, and never will be, one to shut up in the face of something I think is fundamentally wrong.

    Amen brother!

  3. #3
    Registered User
    Join Date
    Oct 2008
    Posts
    17
    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

  4. #4
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    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"?

  5. #5
    Registered User
    Join Date
    Oct 2008
    Posts
    17
    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.

  6. #6
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    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.)

  7. #7
    Ex scientia vera
    Join Date
    Sep 2007
    Posts
    478
    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.
    "What's up, Doc?"
    "'Up' is a relative concept. It has no intrinsic value."

  8. #8
    Registered User
    Join Date
    Oct 2008
    Posts
    17
    Quote Originally Posted by tabstop View Post
    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.

  9. #9
    Registered User
    Join Date
    Oct 2008
    Posts
    17
    Quote Originally Posted by IceDane View Post
    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

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Problems with shared memory shmdt() shmctl()
    By Jcarroll in forum C Programming
    Replies: 1
    Last Post: 03-17-2009, 10:48 PM
  2. Suggestions on this C style code
    By Joelito in forum C Programming
    Replies: 11
    Last Post: 06-07-2007, 03:22 AM
  3. Relate memory allocation in struct->variable
    By Niara in forum C Programming
    Replies: 4
    Last Post: 03-23-2007, 03:06 PM
  4. What's the best memory (RAM) type?
    By Unregistered in forum A Brief History of Cprogramming.com
    Replies: 17
    Last Post: 12-14-2001, 11:37 PM
  5. traping leaking memory
    By null in forum C Programming
    Replies: 5
    Last Post: 10-01-2001, 12:02 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21