Doubly freed objects with SWIG 4.1 and PHPNG Mapscript

[bragef]

MapScript builds fine with SWIG-master (4.1.0-dev) and PHP 8.0.12 but chrashes when running a simple script:
`<?php

$map = new mapObj("/var/gis/mapfiles/demo.map");

$a = $map->getLayer(1);

//to this point is all right

$b = $map->getLayer(1);

//crashing when calling getLayer a second time`

/var/log/apache2/error.log
double free or corruption (!prev) [Wed Nov 03 14:39:22.650920 2021] [core:notice] [pid 19637] AH00051: child pid 19638 exit signal Aborted (6), possible coredump in /etc/apache2

Originally posted by @bjoernboldt in #6201 (comment)

[bragef]

If any of the PHP Mapscript methods mapObj::getLayer(), layerObj::getClass() or classObj::getStyle() are called more than once, the PHP process will crash during cleanup. This happens in both PHP7 and PHP8 when compiled using SWIG-4.1. Likely related to the new PHP object handling in swig/swig/pull/1982, not to the PHP8 support.

Tested on Linux RHEL7.9, with PHP 7.2.24 and PHP 8.0.12 (CLI), Mapserver 7.6.2 and SWIG 4.1.0.

Example script:

<?php
$mapfile = '/usr/local/src/mapserver/mapserver-7.6.2/tests/test.map';

$map = new mapObj($mapfile);

$layer = $map->getLayer(1);
$layer2 = $map->getLayer(1);

## Crash can be prevented by setting thisown=0 to avoid freeing the pointed to layer
# $layer2->thisown = 0;

## These will crash in the same way:
# $class = $layer->getClass(0);
# $class2 = $layer->getClass(0);
# $style = $class->getStyle(0);
# $style2 = $class->getStyle(0);

## This will also crash in the same way
# $newlayer = new layerObj();
# $layernum = $map->insertLayer($newlayer);
# $layer3 = $map->getLayer($layernum);

Result of running script with PHP8:

*** Error in `/usr/local/php8/php-8.0.12/bin/php': double free or corruption (!prev): 0x0000000001bf8350 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x81329)[0x7ffff34d7329]
/usr/local/src/mapserver/mapserver-7.6.4/build-php8/mapscript/phpng/php_mapscriptng.so(layerObj_free_storage+0x57)[0x7fffef93582c]
/usr/local/php8/php-8.0.12/bin/php(zend_objects_store_del+0x1bd)[0xa7ce68]
/usr/local/php8/php-8.0.12/bin/php(rc_dtor_func+0x5b)[0x9a535f]
/usr/local/php8/php-8.0.12/bin/php[0x9a52f4]
/usr/local/php8/php-8.0.12/bin/php(zval_ptr_dtor+0x18)[0x9a54cc]
/usr/local/php8/php-8.0.12/bin/php[0x9bdb8e]
/usr/local/php8/php-8.0.12/bin/php[0x9bdc59]
/usr/local/php8/php-8.0.12/bin/php(zend_hash_reverse_apply+0xd1)[0x9bf973]
/usr/local/php8/php-8.0.12/bin/php[0x990c9a]
/usr/local/php8/php-8.0.12/bin/php(zend_call_destructors+0x3c)[0x9a7c2f]
/usr/local/php8/php-8.0.12/bin/php(php_request_shutdown+0x8e)[0x918240]
/usr/local/php8/php-8.0.12/bin/php[0xa89938]
/usr/local/php8/php-8.0.12/bin/php[0xa8a00f]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7ffff3478555]
/usr/local/php8/php-8.0.12/bin/php[0x43bd39]

gdb backtrace:

(gdb) bt
#0  0x00007ffff348c387 in raise () from /lib64/libc.so.6
#1  0x00007ffff348da78 in abort () from /lib64/libc.so.6
#2  0x00007ffff34cef67 in __libc_message () from /lib64/libc.so.6
#3  0x00007ffff34d7329 in _int_free () from /lib64/libc.so.6
#4  0x00007fffef93582c in layerObj_free_storage (object=0x7fffefc742b8)
    at /usr/local/src/mapserver/mapserver-7.6.4/build-php8/mapscript/phpng/CMakeFiles/php_mapscriptng.dir/mapscriptPHP7_wrap.c:4872
#5  0x0000000000a7ce68 in zend_objects_store_del (object=0x7fffefc742b8) at /usr/local/src/php8/php-8.0.12/Zend/zend_objects_API.c:193
#6  0x00000000009a535f in rc_dtor_func (p=0x7fffefc742b8) at /usr/local/src/php8/php-8.0.12/Zend/zend_variables.c:57
#7  0x00000000009a52f4 in i_zval_ptr_dtor (zval_ptr=0x7fffffffc050) at /usr/local/src/php8/php-8.0.12/Zend/zend_variables.h:44
#8  0x00000000009a54cc in zval_ptr_dtor (zval_ptr=0x7fffffffc050) at /usr/local/src/php8/php-8.0.12/Zend/zend_variables.c:84
#9  0x00000000009bdb8e in _zend_hash_del_el_ex (ht=0x182f1b0 <executor_globals+304>, idx=9, p=0x7fffefc59320, prev=0x0)
    at /usr/local/src/php8/php-8.0.12/Zend/zend_hash.c:1330
#10 0x00000000009bdc59 in _zend_hash_del_el (ht=0x182f1b0 <executor_globals+304>, idx=9, p=0x7fffefc59320)
    at /usr/local/src/php8/php-8.0.12/Zend/zend_hash.c:1353
#11 0x00000000009bf973 in zend_hash_reverse_apply (ht=0x182f1b0 <executor_globals+304>, apply_func=0x990a83 <zval_call_destructor>)
    at /usr/local/src/php8/php-8.0.12/Zend/zend_hash.c:1924
#12 0x0000000000990c9a in shutdown_destructors () at /usr/local/src/php8/php-8.0.12/Zend/zend_execute_API.c:246
#13 0x00000000009a7c2f in zend_call_destructors () at /usr/local/src/php8/php-8.0.12/Zend/zend.c:1208
#14 0x0000000000918240 in php_request_shutdown (dummy=0x0) at /usr/local/src/php8/php-8.0.12/main/main.c:1787
#15 0x0000000000a89938 in do_cli (argc=4, argv=0x1835990) at /usr/local/src/php8/php-8.0.12/sapi/cli/php_cli.c:1111
#16 0x0000000000a8a00f in main (argc=4, argv=0x1835990) at /usr/local/src/php8/php-8.0.12/sapi/cli/php_cli.c:1337

[jmckenna]

@bragef I cannot reproduce this on Windows (MapServer-main, PHP 8.0.12, SWIG 4.1.0-master). Can you also compile MapServer-main and test against this as well? thanks.

[bragef]

Same crash with MapServer-main (7.7-dev):

*** Error in `/usr/local/php8/php-8.0.12/bin/php': double free or corruption (!prev): 0x0000000003937f80 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x81329)[0x7ff6b7628329]
/usr/local/src/mapserver/MapServer/build-test/mapscript/phpng/php_mapscriptng.so(layerObj_free_storage+0x57)[0x7ff6b3b3bbe1]

[jmckenna]

It could be failing silently on Windows too. Thanks for this report, will test more here...

[bragef]

I believe the issue here is that these functions always return a reference to an existing object, but are declared as %newobject in SWIG. As I understand it, the %newobject directive tells the host language that the function returns a newly created object, which may be garbage collected when the (host language) object is destroyed, see http://www.swig.org/Doc4.0/Customization.html#Customization_ownership.

If I remove %newobject from these functions, the code runs without crashing (and with no complaints about memory leakage).

/usr/local/src/mapserver/MapServer/build-test/mapscript/phpng$ git diff
diff --git a/mapscript/swiginc/class.i b/mapscript/swiginc/class.i
index 7e4ce48..b8aa93b 100644
--- a/mapscript/swiginc/class.i
+++ b/mapscript/swiginc/class.i
@@ -213,7 +213,6 @@
   
   // See https://github.com/mapserver/mapserver/issues/548 for more details about the Style methods
 
-  %newobject getStyle;
   /// Return a reference to the :class:`styleObj` at *index* in the styles array
   styleObj *getStyle(int i) {
     if (i >= 0 && i < self->numstyles) {
diff --git a/mapscript/swiginc/layer.i b/mapscript/swiginc/layer.i
index 791f3f8..ffdab16 100644
--- a/mapscript/swiginc/layer.i
+++ b/mapscript/swiginc/layer.i
@@ -305,7 +305,6 @@
             return NULL;
     }
 
-    %newobject getClass;
     /// Fetch the requested class object at *i*. Returns NULL if the class index is out of the legal range. 
     /// The numclasses field contains the number of classes available, and the first class is index 0.
     classObj *getClass(int i) 
diff --git a/mapscript/swiginc/map.i b/mapscript/swiginc/map.i
index 79b4919..4f8d2a8 100644
--- a/mapscript/swiginc/map.i
+++ b/mapscript/swiginc/map.i
@@ -181,7 +181,6 @@
         return msMapSetRotation( self, rotation_angle );
     }
 
-    %newobject getLayer;
     /// Returns a reference to the layer at index *i*.
     layerObj *getLayer(int i) {
       if(i >= 0 && i < self->numlayers) {
@@ -192,7 +191,6 @@
       }
     }
 
-    %newobject getLayerByName;
     /// Returns a reference to the named layer.
     layerObj *getLayerByName(char *name) {
       int i;

[geographika]

Those %newobject directives were added 15 years ago as part of RFC24

Although interestingly the RFC also states:

For the reference count to work all object ownership must be given to SWIG. This is quite different from how it is today. The change however is straightforward because SWIG will acquire object ownership by default and is only a matter of removing all %newobject statements in the swig interface files.

https://mapserver.org/fr/development/rfc/ms-rfc-24.html#always-give-object-ownership-to-swig

[bragef]

Those %newobject directives were added 15 years ago as part of RFC24

I cannot see why they are necessary from RFC-24, the allocated memory is already owned by the containing mapobj (or by a layerobj). A %newobject directive here will just add conflict over which objects owns the memory (if I understand it correctly).

[geographika]

@bragef - yes the code update seems in contradiction to the RFC. Maybe try removing and adding a pull request to see if the Python MapScript test suite continues to pass (it will run on both Windows and Linux).

[rouault]

I'm not knowledgeable of PHP by any means, but I do reproduce the crash with SWIG master and PHP 7.4.3 and 8.1.0
Looking at the generated mapscriptPHP7_wrap.c file between SWIG 4.0.2 (for which PHPNG works with PHP 7.4.3) and SWIG master, I can see that all the custom destructors of our objects are not used with SWIG master and replaced with a regular free(), which obviously cause double free, since our custom destructors are reference counting aware and avoid that. I believe SWIG 4.1.0dev is broken in that respect and the comment at https://github.com/swig/swig/pull/1982/files#diff-577f2e453ebd3b1f48e05052ac488abfd1634f39d439d24a62f1dba8f544c8f1R141-R144 which is the portion where regular free() is called on our mapserver objects instead of custom deallocators confirms that suspicion.

Removing the %newobject directive would be incorrect (RFC24 text is not consistent with implementation, but implementation looks correct to me. As we increase the native reference count in methods returning %newobject, things are consistent) and would cause memory leaks for other languages.

My impression is that the fix should be in SWIG itself. I've filed swig/swig#2108 about that

[rouault]

Side note: beyond the blocking issue with SWIG, I think PHPNG Mapscript itself might be a bit brittle, as there's no implementation of RFC 24 in mapscript/phpng/php7module.i, that is getLayer() should for example make sure that the PHP layer object keeps a reference to the belonging PHP map object (like what is done in javamodule.i, plmodule.i, pymodule.i and csmodule.i), but such issues would appear in more complex scenarios, like getting a layer from a map, dropping references to the map and calling methods on the layer that will access the map

[bragef]

My impression is that the fix should be in SWIG itself. I've filed swig/swig#2108 about that

That seems to be it. I tested with updated swig with swig/swig/pull/2112 applied, and the crashes are gone. Also valgrind reports no lost memory.

[jmckenna]

This also works fine with today's SWIG-master (4.1.0-dev) and PHP 8.1.0, on Windows. (calling the layer a second time, and doing stuff on that, works well)

Closing.