On Dec 1, 2016 12:46 PM, Dmitry Stogov <dmi...@zend.com> wrote:
>
> On Dec 1, 2016 12:21 PM, Bob Weinand <bobw...@hotmail.com> wrote:
> >
> >
> >> Am 01.12.2016 um 08:15 schrieb Dmitry Stogov <dmi...@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....@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 <dmi...@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
> >
> >
