Fixing return value memory leaks upon exceptions in opcode operand freeing View as Markdown

Am 01.12.2016 um 08:15 schrieb Dmitry Stogov dmitry@zend.com:

Hi,

I see, the patch is merged, but I didn't see Nikita's opinion and agreement.
Nikita, did you give a green light to this?

1. The latest changes to FE_RESET (especially in CFG construction) look weird (labels don't match to basic blocks).

Quoting Nikita (Stackoverflow chat):

I can't think of anything where it would cause problems right now -- just a bit uncomfortable with the CFG being inaccurate.

I am open for any better fixes to that, though Nikita had no much better patch immediately ready either.

This is a critical problem.
You broke quite complex part of the code.
I'm going to revert the patch, and then review/rework/recommit it part by part.

Thanks. Dmitry.

2. zend_pre_incdec_overloaded_property() doesn't check "result" for NULL.

My mistake, fixed. (also the test with the use-after-free)

3. Changes in shift_left_function() and shift_right_function() seems useless. Just increase code size. (I may be wrong)

Changes there are to avoid failed bitshift overwriting the original value, i.e. see Zend/tests/shift_003.phpt (in particular when the variable is a string).

4. May be it's better to remove ZEND_VM_UNDEF_RETVAL() and insert ZVAL_UNDEF() before HANDLE_EXCEPTION() manually, only when necessary (eliminating opline->result_type checks)

Possibly, though I see no particular gain in avoiding the checks as these are relatively cold paths anyway, making it easier to not miss that case.

The patch tries to fix few problem at once. It would be better to consider problems separately.

Hmm, what is your strategy when you notice a few related issues

For example, possible memleaks in bitwise functions had to be fixed separately (in 7.0 and above)

Hmm, Nikita said it'd be better to merge it based on master (I originally planned 7.1, but agreed.).

I suppose, Nikita told not to merge the whole patch into 7.1, because it's too risky. (I'm completely agree).

But unrelated minor fixes, just have to be done separately.

I'm currently run "make test TESTS=-m Zend" (this was your job before committing), and depending on found bugs (I already see few failures) we might have to revert the whole patch, including these right fixes.

Thanks. Dmitry.

But feel free to backport the fixes as you think it is needed.

Bob

Thanks. Dmitry.

---

From: Dmitry Stogov
Sent: Thursday, November 24, 2016 2:23:41 PM
To: Nikita Popov
Cc: Bob Weinand; Nikita Popov
Subject: Re: Fixing return value memory leaks upon exceptions in opcode operand freeing

Hi Nikita,

I don't like this solution (looks a bit complex and dangerous), but I don't see another way to fix the problem.

Anyway, after the last fixes, it looks quite well.
I find just two minor problems, and Bob already fixed them, however, I afraid, we might miss more place(s).
Could you please review this once again and give your opinion.

If you are OK, I'm OK as well.

Thanks. Dmitry.

---

From: Nikita Popov nikita.ppv@gmail.com
Sent: Tuesday, November 22, 2016 2:31:45 PM
To: Dmitry Stogov
Cc: Bob Weinand; Nikita Popov
Subject: Re: Fixing return value memory leaks upon exceptions in opcode operand freeing

Hi Bob,

As I understand, one of the idea of the patch is changing responsibility for "result" destruction.
Currently, opcode handlers care about this, your patch proposes to move responsibility to HANDLE_EXCEPTION.

Unfortunately, this doesn't work even for very basic scenarios:

$ USE_ZEND_ALLOC=0 valgrind sapi/cli/php -r '$a=[1,2]; $a + 5;'

==10171== Conditional jump or move depends on uninitialised value(s)
==10171== at 0x8661718: zval_ptr_dtor nogc (zend_variables.h:39)
==10171== by 0x866BC8E: ZEND_HANDLE_EXCEPTION_SPEC_HANDLER (zend_vm_execute.h:1789)
==10171== by 0x866866B: execute_ex (zend_vm_execute.h:429)
==10171== by 0x8668720: zend_execute (zend_vm_execute.h:474)
==10171== by 0x860B06F: zend_eval_stringl (zend_execute_API.c:1093)
==10171== by 0x860B1EB: zend_eval_stringl_ex (zend_execute_API.c:1134)
==10171== by 0x860B248: zend_eval_string_ex (zend_execute_API.c:1145)
==10171== by 0x86D9C09: do_cli (php_cli.c:1021)
==10171== by 0x86DA786: main (php_cli.c:1378)

I afraid, I might find tens more cases in few minutes.
Adding ZVAL_UNDEF(result) in every missing exceptional place doesn't make a lot of sense, adding ZVAL_UNDEF(result) on fast paths make even less sense.

I think, this part of the patch should be removed.

I'm still reviewing...

Thanks. Dmitry.

Good point.

I think this is an issue with the operator functions. The problem is that they don't specify a consistent contract for what they do with the result in case of an exception. If the operator function itself throws, the result is not set. If an exception is thrown during the operation (e.g. because of type conversion), it does populate the result. E.g. this leaks:

set_error_handler(function() { throw new Exception; });
try {
$arr = [];
var_dump($arr . "foo");
} catch (Exception $e) {}

Both cases should behave consistently, one way or another.

Nikita