ext/intl: Fix memory leak in MessageFormatter::format()

[Girgias]

Found while investigating #11614 this is probably present in all versions of PHP and we need a regression test, and probably inform about the proper reason.

@danog do you know which test in https://github.com/zoonru/Valinor might have caused this leak to happen?

[danog]

Quite possibly it's tests/Unit/Utility/String/StringFormatterTest.php

[nielsdos]

I can't reproduce the memory leak. After reverting your patch and executing the same test I don't get leaks with Valgrind.
You seem to have added an else case for "Creating message formatter failed", but this case isn't actually tested in the testcase?
I'm also not sure about that umsg_close, why do we need it? This is the only error-handling place where we use that in PHP AFAICT?

[Girgias]

I can't reproduce the memory leak. After reverting your patch and executing the same test I don't get leaks with Valgrind. You seem to have added an else case for "Creating message formatter failed", but this case isn't actually tested in the testcase? I'm also not sure about that umsg_close, why do we need it? This is the only error-handling place where we use that in PHP AFAICT?

I was hitting it with ASAN:

./configure -C CC=gcc CFLAGS="-DPROFITABILITY_CHECKS=0 -DZEND_RC_DEBUG=1 -DZEND_VERIFY_FUNC_INFO=1 -DZEND_TRACK_ARENA_ALLOC=1 -ggdb3" --enable-address-sanitizer --enable-undefined-sanitizer --enable-debug --enable-tokenizer --enable-opcache --enable-zend-test --enable-dl-test=shared   --enable-pcntl --enable-mbstring --enable-fpm --enable-posix --enable-bcmath --enable-calendar --enable-ctype --enable-exif   --enable-fileinfo --enable-filter --enable-ftp --enable-gd --enable-session --enable-sockets   --enable-sysvmsg --enable-shmop --enable-sysvsem --enable-sysvshm   --enable-dba --with-qdbm --with-cdb --enable-flatfile --enable-inifile --with-lmdb --with-tcadb --with-db4 --with-zip --with-zlib --with-bz2   --with-curl --with-ffi --with-gmp --with-tidy --with-enchant --with-openssl --with-sodium   --with-libxml --enable-dom --enable-simplexml --enable-xml --enable-xmlreader --enable-xmlwriter --with-xsl --enable-soap   --with-sqlite3 --with-mysqli --with-unixODBC --enable-pdo --with-pdo-firebird --with-pdo-mysql --with-pdo-pgsql --with-pdo-sqlite --with-pgsql   --with-iconv --enable-phar --with-pspell --with-readline --enable-intl

And the following test command:

make -j22 && make test TEST_PHP_ARGS="--asan -q -j22 -d opcache.jit=0 -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0 -d opcache.jit_buffer_size=1M -x" TESTS="ext/intl/tests/"

I think part of the reason why we don't normally use that, is that the errors will get and formatter will get closed/destructed when the object's refcount hits 0 as those things are stored within the object.

However, this method is static and creates an "object" on the fly which we were not freeing.

The ASAN stack traced showed the malloc happening in ICU 72 for me.

[nielsdos]

Ah, I tried with Valgrind and couldn't reproduce. ASAN lets me reproduce it.
Code looks good and does indeed fix it.