ID: 26847
User updated by: nutbar at innocent dot com
Reported By: nutbar at innocent dot com
-Status: Bogus
+Status: Open
Bug Type: Mail related
Operating System: any - source code issue
PHP Version: 4.3.4
New Comment:
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).
Previous Comments:
------------------------------------------------------------------------
[2004-01-08 16:07:29] 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 15:38:41] [EMAIL PROTECTED]
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 15:34:23] 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:31:12] 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 13:22:55] 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.
------------------------------------------------------------------------
--
Edit this bug report at http://bugs.php.net/?id=26847&edit=1
