php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #26847 memory leak with to_r and subject_r in mail() function
Submitted: 2004-01-08 13:22 UTC Modified: 2004-01-08 20:32 UTC
From: nutbar at innocent dot com Assigned:
Status: Closed Package: Mail related
PHP Version: 4.3.4 OS: any - source code issue
Private report: No CVE-ID:
 [2004-01-08 13:22 UTC] nutbar at innocent dot com
Description:
------------
In the actual source code for the PHP mail() function (ext/standard/mail.c), it sets some variables up to hold the To: and Subject: headers up, and other stuff.

The problem is that if you look at the initial code that checks if the "to_len" count is greater than 0, it duplicates the "to" string to "to_r" and does some stuff to it.

It does the same sort of thing with subject_len, subject, and subject_r in the exact same fashion.

After the new to_r and subject_r strings are used, it goes to free them, but it does an if () test to see if it should or not - the if test compares to_len and subject_len to see if they are greater than 0 and if so, efree()'s them.

The problem is that in the code that does stuff with to_r and subject_r, there are for loops which decrement to_len and subject_len so it can walk the strings.  By doing this, you bring the to_len and subject_len variables to 0, thus nothing is ever efree()'d in the end, and you've got a memory leak.

The leak is small and not noticable typically, but with mass mailing scripts that loop using mail(), it could be huge.

Reproduce code:
---------------
See mail.c - lines 106 to 113, 129 to 136, and then 160 to 165.

Actual result:
--------------
I have not tested for an actual memory leak by calling mail() in a loop - I was just going to write my own mail() function and was using the code in mail.c to do it with, and came across this.

If this is a false report, I am sorry, but I do believe it's real.

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2004-01-08 15:31 UTC] nutbar at innocent dot com
By the way, very simple fix:

Add this to the variable declarations:

int j = 0;

Then the lines of code as mentioned before, but fixed:

        if (to_len > 0) {
                to_r = estrndup(to, to_len);

                for (j = to_len; j > 0; j--) {
                        if (!isspace((unsigned char) to_r[j - 1])) {
                                break;
                        }

                        to_r[j - 1] = '\0';
                }



and:

        if (subject_len > 0) {
                subject_r = estrndup(subject, subject_len);

                for (j = subject_len; j > 0; j--) {
                        if (!isspace((unsigned char) subject_r[j - 1])) {
                                break;
                        }

                        subject_r[j - 1] = '\0';
                }


I just initialized j in the for loop to be the value of to_len and subject_len, then I use j everywhere rather than to_len or subject_len so that they stay unmodified.
 [2004-01-08 15:34 UTC] nutbar at innocent dot com
I guess an alternate fix would also be when the efree()'s are called.  If you init all your char *'s to NULL, then you can simply do:

if (to_r != NULL) {
     efree(to_r);
}

if (subject_r != NULL) {
     efree(subject_r);
}
 [2004-01-08 15:38 UTC] iliaa@php.net
Impossible leak. 
        if (to_len > 0) { 
                efree(to_r); 
        } 
        if (subject_len > 0) { 
                efree(subject_r); 
        } 
Is what the code in CVS does. If to_len or subject_len are 
< 1 then no allocation happens in the 1st place. 
 [2004-01-08 16:07 UTC] nutbar at innocent dot com
I know they check to_len and subject_len - that's not really the problem.

The problem is that the for loops above that decrement to_len and subject_len - thus modifying them from their original values.

to_len and subject_len will always be 0, even if they weren't 0 to begin with.  Do you see what I'm referring to?
 [2004-01-08 16:17 UTC] nutbar at innocent dot com
Here, maybe this will help a bit...  Here it assigns values to to_len and subject_len (among others):

        if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "sss|ss",
                                                          &to, &to_len,
                                                          &subject, &subject_len,
                                                          &message, &message_len,
                                                          &headers, &headers_len,
                                                          &extra_cmd, &extra_cmd_len
                                                          ) == FAILURE) {
                return;
        }


Then, they check to_len and do stuff if it's greater than 0:

        if (to_len > 0) {
                to_r = estrndup(to, to_len);
                for (; to_len; to_len--) {
                        if (!isspace((unsigned char) to_r[to_len - 1])) {
                                break;
                        }
                        to_r[to_len - 1] = '\0';
                }
...


Do you see the for loop in there... this one:

                for (; to_len; to_len--) {...}

It is modifying to_len itself, which means that to_len, although was NOT 0 to begin with (and thus, to_r was estrndup()'d and we have to efree() it later), but IS 0 in the end once the for loop is finished.

Either the for loop must be changed to not modify to_len, or the efree() statement must be changed to test to_r, not to_len.

Or am I just really out of my mind?  I'm not anywhere near as good as you programmers, but this seems to be sticking out for me quite a bit (I'm not trying to come off rude!  I just think I found something here and it's not bogus).
 [2004-01-08 17:14 UTC] nutbar at innocent dot com
Ok, maybe I am wrong?

I wrote a PHP script which *should* leak memory if this is indeed not efree()'ing stuff, but it doesn't seem to.

I noticed it would only potentially (if I was right!) leak ram if say, the subject was entirely space characters, so I made a script that called mail() 1000 times roughly, and made a subject that was all spaces that was 1k long - it should have set me off by 1mb, but instead I see nothing of the sort.

I'm running the script from the command line (CGI, not CLI though!) if it makes any difference (I don't believe it does).

Either way, I can't seem to leak the ram, so I guess I was wrong - but can someone explain to me why it wouldn't?  What am I missing here that wouldn't allow it to leak ram?
 [2004-01-08 20:32 UTC] iliaa@php.net
This bug has been fixed in CVS.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.

The bug was real, you were right. The reason you did not 
see the leak is because Zend's memory manager automatically 
handles such leaks. If you were to compile a debug build of 
PHP you'd see some leak messages printed. 
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Fri Apr 18 23:01:58 2014 UTC