Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:97244
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 104.47.36.120 as permitted sender)
To: Dmitry Stogov <dmitry@zend.com>
CC: Bob Weinand <bobwei9@hotmail.com>, "nikita.ppv@gmail.com"
	<nikita.ppv@gmail.com>, PHP Internals <internals@lists.php.net>
Thread-Topic: Fixing return value memory leaks upon exceptions in opcode
 operand freeing
Thread-Index: AQHSTHhMKl09Pv6UMkiT9pt1G+CKYA==
Date: Fri, 2 Dec 2016 08:53:46 +0000
Message-ID: <MWHPR02MB24777D70519AA451320799E1BF8E0@MWHPR02MB2477.namprd02.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: zend.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: multipart/alternative;
	boundary="_000_MWHPR02MB24777D70519AA451320799E1BF8E0MWHPR02MB2477namp_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Dec 2016 08:53:46.0619
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR02MB2480
Subject: Re: Fixing return value memory leaks upon exceptions in opcode
 operand freeing
From: dmitry@zend.com (Dmitry Stogov)

--_000_MWHPR02MB24777D70519AA451320799E1BF8E0MWHPR02MB2477namp_
Content-Type: text/plain; charset="koi8-r"
Content-Transfer-Encoding: quoted-printable

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 agre=
ement.
> >> Nikita, did you give a green light to this?
> >>
> >> 1) The latest changes to FE_RESET (especially in CFG construction) loo=
k 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 bette=
r 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 p=
art.

Thanks. Dmitry.

> >
> >> 2) zend_pre_incdec_overloaded_property() doesn't check "result" for NU=
LL.
> >
> >
> > My mistake, fixed. (also the test with the use-after-free)
> >
> >> 3) Changes in shift_left_function() and shift_right_function() seems u=
seless. Just increase code size. (I may be wrong)
> >
> >
> > Changes there are to avoid failed bitshift overwriting the original val=
ue, 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 (eliminati=
ng opline->result_type checks)
> >
> >
> > Possibly, though I see no particular gain in avoiding the checks as the=
se 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 cons=
ider 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 se=
parately (in 7.0 and above)
> >
> >
> > Hmm, Nikita said it'd be better to merge it based on master (I original=
ly 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=3D-m Zend" (this was your job before c=
ommitting), and depending on found bugs (I already see few failures) we mig=
ht 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 opcod=
e 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 opcod=
e 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 responsibil=
ity for "result" destruction.
> >>> Currently, opcode handlers care about this, your patch proposes to mo=
ve responsibility to HANDLE_EXCEPTION.
> >>>
> >>> Unfortunately, this doesn't work even for very basic scenarios:
> >>>
> >>> $ USE_ZEND_ALLOC=3D0 valgrind sapi/cli/php -r '$a=3D[1,2]; $a + 5;'
> >>>
> >>> =3D=3D10171=3D=3D Conditional jump or move depends on uninitialised v=
alue(s)
> >>> =3D=3D10171=3D=3D    at 0x8661718: _zval_ptr_dtor_ nogc (zend_variabl=
es.h:39)
> >>> =3D=3D10171=3D=3D    by 0x866BC8E: ZEND_HANDLE_EXCEPTION_SPEC_HANDLER=
 (zend_vm_execute.h:1789)
> >>> =3D=3D10171=3D=3D    by 0x866866B: execute_ex (zend_vm_execute.h:429)
> >>> =3D=3D10171=3D=3D    by 0x8668720: zend_execute (zend_vm_execute.h:47=
4)
> >>> =3D=3D10171=3D=3D    by 0x860B06F: zend_eval_stringl (zend_execute_AP=
I.c:1093)
> >>> =3D=3D10171=3D=3D    by 0x860B1EB: zend_eval_stringl_ex (zend_execute=
_API.c:1134)
> >>> =3D=3D10171=3D=3D    by 0x860B248: zend_eval_string_ex (zend_execute_=
API.c:1145)
> >>> =3D=3D10171=3D=3D    by 0x86D9C09: do_cli (php_cli.c:1021)
> >>> =3D=3D10171=3D=3D    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 t=
hat they don't specify a consistent contract for what they do with the resu=
lt in case of an exception. If the operator function itself throws, the res=
ult is not set. If an exception is thrown during the operation (e.g. becaus=
e of type conversion), it *does* populate the result. E.g. this leaks:
> >>
> >> set_error_handler(function() { throw new Exception; });
> >> try {
> >>     $arr =3D [];
> >>     var_dump($arr . "foo");
> >> } catch (Exception $e) {}
> >>
> >> Both cases should behave consistently, one way or another.
> >>
> >> Nikita
> >
> >

--_000_MWHPR02MB24777D70519AA451320799E1BF8E0MWHPR02MB2477namp_--