Re: Fixing return value memory leaks upon exceptions in opcode operand freeing

On Dec 1, 2016 12:46 PM, Dmitry Stogov <dmitry@zend.com> wrote: > > On Dec 1, 2016 12:21 PM, Bob Weinand <bobwei9@hotmail.com> wrote: > > > > > >> 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 > >> > >> On Tue, Nov 22, 2016 at 8:24 AM, Dmitry Stogov <dmitry@zend.com> wrote: > >>> > >>> 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 > > > >