Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:118336 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 77806 invoked from network); 1 Aug 2022 10:59:38 -0000 Received: from unknown (HELO php-smtp4.php.net) (45.112.84.5) by pb1.pair.com with SMTP; 1 Aug 2022 10:59:38 -0000 Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 5089F1804BE for ; Mon, 1 Aug 2022 05:59:10 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,HTML_MESSAGE, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE, SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.2 X-Spam-ASN: AS15169 209.85.128.0/17 X-Spam-Virus: No X-Envelope-From: Received: from mail-pf1-f181.google.com (mail-pf1-f181.google.com [209.85.210.181]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Mon, 1 Aug 2022 05:59:09 -0700 (PDT) Received: by mail-pf1-f181.google.com with SMTP id u133so3868119pfc.10 for ; Mon, 01 Aug 2022 05:59:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=62+Z/wkpV7PdiRCK8S/8Gs3rw+YAy3oRDvlC8C1hTJU=; b=nkISWYpI4PwknaTfabvHt13Ccrgt8aJZV43ulfnC5qHm9RqYQdu+pFOUAzxdXRd57l YKaylbj7Xh18rlxFy6Ala/W52w9v/OEAycFN3YuLiWurv4xp16rKkk/JfaMQWsAg2uqo btAIqY+/vZqUZhrb7/9sbWci93guJsZ5rMuIc9hZY3ENvnbxer+WS27HNw18/HsSjLDg 0D39PTZNa1luaGjU15sOyxtdREcDbtRYOY6MYFc9ByFKxDkF4kgOEkNStOfKKlJIk9ea LOPXchmZeS77QxgB5+Lfvbw4zb0saUzCqxSP4w7P5FPzdExaqRjmiI60nghFbmEOtd5N ZXHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=62+Z/wkpV7PdiRCK8S/8Gs3rw+YAy3oRDvlC8C1hTJU=; b=AotWNH/XoZnWv7Sb0AZkrXGSgO9wgDcG17otjkLpo4x4KXH7e11OqIxBPWJk/QQNW6 fMwQrprd2Pk7ERKAsQDF/BgrZ7XHVDgTEyoDrMSJC2wuxqiEY0sORoDRd29Evds/7Rhc D4mbrW5EzhlYnphRApTu39JUn3E5zs1Sa2v1p1Pp+ztf9qIm86KxNeBZkawUyGCoYBO4 v3ZadDdPk5UecFZHUcc9U9Tp0zav7WYTSN65z+6QdKOXfV4BocF4flieImR/TLZY8Jlz 5dDPWtu9YU4uwTXODtccTruT2ZIFYaI/vwRvclRV2tfXy3DqPkpktB28alv39w4E7gwm genA== X-Gm-Message-State: AJIora8xjv91RaLm8nTirv/fvyB77sysx0r7cGxM3b2VxhHuuT71JLn0 5TU/AS7qNf+bbdidxA36uXfOY0DFURLIt5sZWOOahK4vgHE= X-Google-Smtp-Source: AGRyM1snNFvgDlMnwkyjmwZLtYm6TNAHNWYUlFXA23AI+FR0FfvBh6Zf8R0BnVxPI+hFEzG6oQ8E9ga+TYdGfSJ728c= X-Received: by 2002:a63:d54:0:b0:416:73d:d5cf with SMTP id 20-20020a630d54000000b00416073dd5cfmr13472463pgn.579.1659358748577; Mon, 01 Aug 2022 05:59:08 -0700 (PDT) MIME-Version: 1.0 References: <302000df-5c3f-a86c-a608-2a45d2726ab1@bastelstu.be> In-Reply-To: <302000df-5c3f-a86c-a608-2a45d2726ab1@bastelstu.be> Date: Mon, 1 Aug 2022 13:58:57 +0100 Message-ID: To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= Cc: PHP internals Content-Type: multipart/alternative; boundary="000000000000c3933605e52d90eb" Subject: Re: [PHP-DEV] What type of Exception to use for unserialize() failure? From: george.banyard@gmail.com ("G. P. B.") --000000000000c3933605e52d90eb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 29 Jul 2022 at 18:17, Tim D=C3=BCsterhus wrote: > Hi! > > I'm currently in the process of cleaning up the error handling of the > new ext/random and now came across the implementation of __unserialize(). > > Looking through the source code for existing examples I found that the > following is used: > > - ext/gmp: Exception > - ext/hash: Exception > - ext/date: Error > - ext/spl: UnexpectedValueException > - ext/random: Currently Exception. > > =E2=80=A6 all of those with different error messages. unserialize() itsel= f will > emit a regular 'Notice' (yes, you read that right) when encountering > garbage data. > > In the end I've opted to follow ext/date's lead and created a PR > changing ext/random to use an Error, as unserialization failures likely > mean that you are unserializing untrusted data which you should not do > in the first place. Also any errors during unserializing are not really > recoverable: You can't go and fix up the input string. > > The PR can be found here: > > https://github.com/php/php-src/pull/9185 > > During review cmb noted that using an 'Error' here might not be the best > choice, as while it is likely to be a programmer error if unserializing > fails, we do not want to educate users to catch(Error). > I probably have a very different philosophy on this as I don't mind users catching Errors, but it probably be mostly done by Framework/libraries doing some funky stuff. And passing user input which can fail to unserialize is not really a programming error and something that can be "expected" to happen, I agree that an Exception does make more sense here. > As the existing behavior already is inconsistent and the Notice really > should be upgraded to something =E2=80=A6 more alarming in the future, I'= m > putting this topic up for discussion on the list. > > My suggested path forward would be: > > For 8.2: > > - Update ext/random to use the ext/date wording (I like that one best): > > "Invalid serialization data for object." > > - Revert the change from Exception to Error in my ext/random PR #9185. > Those make sense to me > - Update ext/date to Exception (the Error for __unserialize() was added > in 8.2, but there already was an Error for __wakeup() before). > I'm not a super fan of this, both __wakeup and __unserialize should use the same one. As far as I can tell the reason __wakeup() is still defined is if someone called it explicitly. But in any case if someone was handing an unserialisation failure (that was handled by __wakeup prior to 8.2) and caugh an Error explicitly this would now break. Thus I'd prefer handling this with the rest after 8.2 > For whatever comes after 8.2: > > - Add a new UnserializationFailureException extends Exception (or > whatever color the bikeshed should have). Any existing catch blocks > should still match, as Exception is more general. It would break for > ext/spl, though. Unless UnserializationFailureException extends > UnexpectedValueException. > As we would be breaking BC with ext/date anyway I think having it just extend Exception would be fine > - Add a helper function that throws this Exception with a consistent > wording. > - Upgrade unserialize() from Notice to UnserializationFailureException. > Notice to Exception might be a big jump, E_WARNING definitely an then promote to an Exception in 9.0 is probably more in line with what we did for 7.x/8.0 George P. Banyard --000000000000c3933605e52d90eb--