#26847 [Opn]: memory leak with to_r and subject_r in mail() function
ID: 26847
User updated by: nutbar at innocent dot com
Reported By: nutbar at innocent dot com
Status: Open
Bug Type: Mail related
Operating System: any - source code issue
PHP Version: 4.3.4
New Comment:
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.
Previous Comments:
[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=26847edit=1
#26847 [Opn]: memory leak with to_r and subject_r in mail() function
ID: 26847
User updated by: nutbar at innocent dot com
Reported By: nutbar at innocent dot com
Status: Open
Bug Type: Mail related
Operating System: any - source code issue
PHP Version: 4.3.4
New Comment:
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);
}
Previous Comments:
[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=26847edit=1
#26847 [Opn]: memory leak with to_r and subject_r in mail() function
ID: 26847
User updated by: nutbar at innocent dot com
Reported By: nutbar at innocent dot com
Status: Open
Bug Type: Mail related
Operating System: any - source code issue
PHP Version: 4.3.4
New Comment:
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?
Previous Comments:
[2004-01-08 16:17:25] 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 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
