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

[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-2024 The PHP Group All rights reserved.	Last updated: Mon Dec 30 14:01:28 2024 UTC