PHP :: Bug #40392 :: memory leaks in PHP milter SAPI and proposed fix

Bug #40392	memory leaks in PHP milter SAPI and proposed fix
Submitted:	2007-02-07 19:56 UTC	Modified:	2007-03-28 10:10 UTC
From:	tuxracer69 at gmail dot com	Assigned:
Status:	Closed	Package:	Unknown/Other Function
PHP Version:	5CVS-2007-02-07 (snap)	OS:	Linux 2.6.17-2-686
Private report:	No	CVE-ID:	None

View Developer Edit

Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.

Password:

Status:
Package:
Bug Type:
Summary:
From:	tuxracer69 at gmail dot com
New email:
PHP Version:		OS:

New Comment:

[2007-02-07 19:56 UTC] tuxracer69 at gmail dot com

Description:
------------
After compiling the last CVS snapshot (5.2) 
./configure \
--with-pgsql --with-curl --with-xml \
--enable-sockets --with-zlib --with-bz2 \
--with-iconv --enable-track-vars --enable-sysvsem \
--enable-sysvshm --enable-inline-optimization \
--with-curl --with-regex=system --with-png \
--enable-discard-path --enable-dbx --enable-memory-limit \
--disable-debug --disable-rpath --with-openssl \
--enable-exif --enable-mbstring  \
--with-xsl=/usr/lib \
--with-milter --disable-cli --disable-cgi --enable-debug

and launching the example milter I was getting the memory leaks warnings below.

-----------------
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(304) :  Freeing 0xA5E46184 (44 bytes), script=-
/usr/local/php5.2-200702061930/Zend/zend_API.c(819) : Actual location (location was relayed)
Last leak repeated 1 time
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(196) :  Freeing 0xA5E464
70 (10 bytes), script=-
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(346) :  Freeing 0xA5E466
C4 (3 bytes), script=-
Last leak repeated 5 times
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/Zend/zend_API.c(1193) :  Freeing 0xA5E466F8 (16 bytes), script=-
Last leak repeated 2 times
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(229) :  Freeing 0xA5E468A8 (10 bytes), script=-
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/Zend/zend_API.c(1194) :  Freeing 0xA5E468E4 (17 bytes), script=-
Last leak repeated 2 times
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/Zend/zend_API.c(1196) :  Freeing 0xA5E46958 (35 bytes), script=-
/usr/local/php5.2-200702061930/Zend/zend_hash.c(388) : Actual location (location was relayed)
Last leak repeated 2 times
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(411) :  Freeing 0xA5E46E
D8 (43 bytes), script=-
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(347) :  Freeing 0xA5E470
50 (40 bytes), script=-
Last leak repeated 5 times
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(264) :  Freeing 0xA5E49A
58 (32 bytes), script=-
/usr/local/php5.2-200702061930/Zend/zend_alloc.c(1917) : Actual location (location was relayed)
Last leak repeated 1 time
[Wed Feb  7 10:20:03 2007]  Script:  '-'
/usr/local/php5.2-200702061930/sapi/milter/php_milter.c(192) :  Freeing 0xA5E505E8 (16 bytes), script=-
=== Total 29 memory leaks detected ===
---------------


The TODO file in the distribution mention such leaks.



Reproduce code:
---------------
the milter.php in the milter sapi distribution 

Expected result:
----------------
no memory leaks

Actual result:
--------------
a lot of memory leaks warnings.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports

[2007-02-07 20:03 UTC] tuxracer69 at gmail dot com

after digging in the cli SAPI code I tried using zval_ptr_dtor instead of the FREE_ZVAL macro.

This resolved the issue (at least I do not see the warnings anymore).

Could somebody confirm this?
Thanks
Alex

----here is my diff---


diff php_milter.c php_milter.c.dist
205c205
<       zval_ptr_dtor(param);
---
> 
238d237
<       zval_ptr_dtor(param);
239a239
>       FREE_ZVAL(param[0]);
278d277
<       zval_ptr_dtor(param);
279a279
>       FREE_ZVAL(param[0]);
319,320c319
<       zval_ptr_dtor(param);
< 
---
>       FREE_ZVAL(param[0]);
358,360c357,358
<       zval_ptr_dtor(&param[0]);
<       zval_ptr_dtor(&param[1]);
< 
---
>       FREE_ZVAL(param[0]);
>       FREE_ZVAL(param[1]);
423,424c421
<       zval_ptr_dtor(param);
< 
---
>       FREE_ZVAL(param[0]);

[2007-02-07 20:12 UTC] tony2001@php.net

>Could somebody confirm this?
Yes.

This SAPI is orphaned for years, so I would actually like to move it to PECL (or anywhere else, just because we cannot guarantee if it works at all). Also looking at the code I can see other problems, one more reason to remove this SAPI.

[2007-02-07 22:10 UTC] tuxracer69 at gmail dot com

Well, the goal of the sapi is to call PHP code based on SMTP commands a client sends to a sendmail server, and in that respect I would say 'it works'.
Although I did not put the code in a production server, (that's what I eventually plan to do) I would say that the functions do what they are expected to do.
The memory leaks were really ugly.
I have other warnings at compile time I could have a look at.

I think there is definitely an interest in processing mail with PHP as after a previous bug we fixed in the milter sapi (Bug #40083) I got mails of other developers asking questions about the stability of the sapi.

Perhaps moving this code to PECL would slow down its development at a moment where interest is growing and a (relatively) small effort may take the code up to date.

thanks
Alex

[2007-02-09 07:18 UTC] tuxracer69 at gmail dot com

Unified diff for the code change above:

diff -u php_milter.c.dist php_milter.c
--- php_milter.c.dist   2007-02-07 10:18:03.000000000 +0000
+++ php_milter.c        2007-02-07 19:41:54.000000000 +0000
@@ -202,7 +202,7 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
-
+       zval_ptr_dtor(param);
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
        }
@@ -235,8 +235,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -275,8 +275,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -316,7 +316,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -354,8 +355,9 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
-       FREE_ZVAL(param[1]);
+       zval_ptr_dtor(&param[0]);
+       zval_ptr_dtor(&param[1]);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -418,7 +420,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);

[2007-02-09 08:10 UTC] tuxracer69 at gmail dot com

a longer patch aimed at fixing besides the mem leaks the compile warnings (type cast, declarations, parenthesis).


diff -u php_milter.c.dist php_milter.c
--- php_milter.c.dist   2007-02-07 10:18:03.000000000 +0000
+++ php_milter.c        2007-02-09 08:01:01.000000000 +0000
@@ -63,6 +63,8 @@
 
 #include "libmilter/mfapi.h"
 
+#include "php_getopt.h"
+
 #define OPTSTRING "ac:d:Def:hnp:vVz:?"
 #define MG(v)  TSRMG(milter_globals_id, zend_milter_globals *, v)
 
@@ -202,7 +204,7 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
-
+       zval_ptr_dtor(param);
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
        }
@@ -235,8 +237,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -275,8 +277,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -316,7 +318,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -354,8 +357,9 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
-       FREE_ZVAL(param[1]);
+       zval_ptr_dtor(&param[0]);
+       zval_ptr_dtor(&param[1]);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -408,7 +412,7 @@
        INIT_PZVAL(param[0]);
 
        ZVAL_STRING(&function_name, "milter_body", 0);
-       ZVAL_STRINGL(param[0], bodyp, len, 1);
+       ZVAL_STRINGL(param[0], (char*)bodyp, len, 1); /*alex*/
 
        /* set the milter context for possible use in API functions */
        MG(ctx) = ctx;
@@ -418,7 +422,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -548,7 +553,7 @@
        if (MG(state) != MLFI_INIT) {
                php_error(E_WARNING, NOT_INIT, get_active_function_name(TSRMLS_C));
        } else if (zend_parse_parameters(1 TSRMLS_CC, "l", &flags) == SUCCESS) {
-               flags = flags & SMFIF_ADDHDRS|SMFIF_CHGHDRS|SMFIF_CHGBODY|SMFIF_ADDRCPT|SMFIF_DELRCPT;
+               flags = flags & (SMFIF_ADDHDRS|SMFIF_CHGHDRS|SMFIF_CHGBODY|SMFIF_ADDRCPT|SMFIF_DELRCPT);
                smfilter.xxfi_flags = flags;
        }
 }
@@ -703,7 +708,7 @@
        if (MG(state) != MLFI_EOM) {
                php_error(E_WARNING, NOT_EOM, get_active_function_name(TSRMLS_C));
        } else if (zend_parse_parameters(1 TSRMLS_CC, "s", &body, &len) == SUCCESS) {
-               if (smfi_replacebody(MG(ctx), body, len) == MI_SUCCESS) {
+               if (smfi_replacebody(MG(ctx), (u_char*)body, len) == MI_SUCCESS) {
                        RETURN_TRUE;
                }
        }
@@ -732,6 +737,7 @@
 
        MG(state) = MLFI_NONE;
        MG(initialized) = 0;
+       return SUCCESS;
 }
 /* }}} */
 
@@ -920,9 +926,7 @@
 /* temporary locals */
        int orig_optind=ap_php_optind;
        char *orig_optarg=ap_php_optarg;
-       char *arg_free=NULL, **arg_excp=&arg_free;
        int interactive=0;
-       char *exec_direct=NULL;
        char *param_error=NULL;
 /* end of temporary locals */
 
@@ -1099,7 +1103,7 @@
 
                openlog("php-milter", LOG_PID, LOG_MAIL);
 
-               if (exit_status = mlfi_init()) {
+               if (exit_status == mlfi_init()) {
                        syslog(1, "mlfi_init failed.");
                        exit(exit_status);
                }

[2007-02-09 08:52 UTC] tuxracer69 at gmail dot com

oops the exit_status was broken, should be:

diff -u php_milter.c.dist php_milter.c
--- php_milter.c.dist   2007-02-07 10:18:03.000000000 +0000
+++ php_milter.c        2007-02-09 08:48:03.000000000 +0000
@@ -63,6 +63,8 @@
 
 #include "libmilter/mfapi.h"
 
+#include "php_getopt.h"
+
 #define OPTSTRING "ac:d:Def:hnp:vVz:?"
 #define MG(v)  TSRMG(milter_globals_id, zend_milter_globals *, v)
 
@@ -202,7 +204,7 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
-
+       zval_ptr_dtor(param);
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
        }
@@ -235,8 +237,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -275,8 +277,8 @@
        call_user_function(CG(function_table), NULL, &function_name, &retval, 1, param TSRMLS_CC);
 
        MG(state) = MLFI_NONE;
+       zval_ptr_dtor(param);
 
-       FREE_ZVAL(param[0]);
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -316,7 +318,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -354,8 +357,9 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
-       FREE_ZVAL(param[1]);
+       zval_ptr_dtor(&param[0]);
+       zval_ptr_dtor(&param[1]);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -408,7 +412,7 @@
        INIT_PZVAL(param[0]);
 
        ZVAL_STRING(&function_name, "milter_body", 0);
-       ZVAL_STRINGL(param[0], bodyp, len, 1);
+       ZVAL_STRINGL(param[0], (char*)bodyp, len, 1); /*alex*/
 
        /* set the milter context for possible use in API functions */
        MG(ctx) = ctx;
@@ -418,7 +422,8 @@
 
        MG(state) = MLFI_NONE;
 
-       FREE_ZVAL(param[0]);
+       zval_ptr_dtor(param);
+
 
        if (Z_TYPE(retval) == IS_LONG) {
                return Z_LVAL(retval);
@@ -548,7 +553,7 @@
        if (MG(state) != MLFI_INIT) {
                php_error(E_WARNING, NOT_INIT, get_active_function_name(TSRMLS_C));
        } else if (zend_parse_parameters(1 TSRMLS_CC, "l", &flags) == SUCCESS) {
-               flags = flags & SMFIF_ADDHDRS|SMFIF_CHGHDRS|SMFIF_CHGBODY|SMFIF_ADDRCPT|SMFIF_DELRCPT;
+               flags = flags & (SMFIF_ADDHDRS|SMFIF_CHGHDRS|SMFIF_CHGBODY|SMFIF_ADDRCPT|SMFIF_DELRCPT);
                smfilter.xxfi_flags = flags;
        }
 }
@@ -703,7 +708,7 @@
        if (MG(state) != MLFI_EOM) {
                php_error(E_WARNING, NOT_EOM, get_active_function_name(TSRMLS_C));
        } else if (zend_parse_parameters(1 TSRMLS_CC, "s", &body, &len) == SUCCESS) {
-               if (smfi_replacebody(MG(ctx), body, len) == MI_SUCCESS) {
+               if (smfi_replacebody(MG(ctx), (u_char*)body, len) == MI_SUCCESS) {
                        RETURN_TRUE;
                }
        }
@@ -732,6 +737,8 @@
 
        MG(state) = MLFI_NONE;
        MG(initialized) = 0;
+    return SUCCESS;
+
 }
 /* }}} */
 
@@ -920,9 +927,7 @@
 /* temporary locals */
        int orig_optind=ap_php_optind;
        char *orig_optarg=ap_php_optarg;
-       char *arg_free=NULL, **arg_excp=&arg_free;
        int interactive=0;
-       char *exec_direct=NULL;
        char *param_error=NULL;
 /* end of temporary locals */
 
@@ -1099,7 +1104,7 @@
 
                openlog("php-milter", LOG_PID, LOG_MAIL);
 
-               if (exit_status = mlfi_init()) {
+               if ((exit_status = mlfi_init())) {
                        syslog(1, "mlfi_init failed.");
                        exit(exit_status);
                }

[2007-03-08 13:52 UTC] tuxracer69 at gmail dot com

Hi,
Just to follow up on this bug, the patch above has now been running in a production server for a month and the whole setup  sendmail+phpmilter was stable.
Alex

[2007-03-09 10:33 UTC] tony2001@php.net

Please upload the patch somewhere (applying copy/pasted patches is non-trivial).
Why did you need to include php_getopt.h header, I wonder?

[2007-03-09 11:50 UTC] tuxracer69 at gmail dot com

Hi Tony,

I put the patch at the URL below:

http://atpic.com/bug40392.patch.txt

I included the php_getopt.h because the compiler complained about it.

Thanks
Alex

[2007-03-28 10:10 UTC] tony2001@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.

	php.net \| support \| documentation \| report a bug \| advanced search \| search howto \| statistics \| random bug \| login
go to bug id or search bugs for


Copyright © 2001-2025 The PHP Group All rights reserved.	Last updated: Tue Jan 28 02:01:30 2025 UTC