Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Re: [PHP-CVS] com php-src: ext/sodium: throw exceptions instead of errors: ext/sodium/libsodium.c

Posted by Sebastian Bergmann 
Am 28.11.2017 um 13:56 schrieb Frank Denis:
> Commit: c05cbd1e775fa69ed9939796a908390f2bfb4459
> Author: Frank Denis <[email protected]> Tue, 28 Nov 2017 13:56:11 +0100
> Parents: c219991c77e4c68f7d62963e18a1da778de0bbe0
> Branches: PHP-7.2
>
> Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=c05cbd1e775fa69ed9939796a908390f2bfb4459
>
> Log:
> ext/sodium: throw exceptions instead of errors

I am very much in favor of this change. However, making such a change in
PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Am 28.11.2017 um 14:17 schrieb Sebastian Bergmann <[email protected]>:
> Am 28.11.2017 um 13:56 schrieb Frank Denis:
>> Commit: c05cbd1e775fa69ed9939796a908390f2bfb4459
>> Author: Frank Denis <[email protected]> Tue, 28 Nov 2017 13:56:11 +0100
>> Parents: c219991c77e4c68f7d62963e18a1da778de0bbe0
>> Branches: PHP-7.2
>>
>> Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=c05cbd1e775fa69ed9939796a908390f2bfb4459
>>
>> Log:
>> ext/sodium: throw exceptions instead of errors
>
> I am very much in favor of this change. However, making such a change in
> PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?

I'm wary of turning errors into exceptions because code which was returning a result with libsodium before (either PECL or PHP 7.2.0) - even though it was generating an E_WARNING/E_ERROR - now suddenly aborts with an exception.

Even if it was put into 7.2.0 it would break code written using the PECL module.
(Side-note: I see that this case was already upgrade from E_WARNING to E_ERROR from PECL to PHP 7.2.0. But the program keeps running with E_ERROR so that's IMHO an acceptable change)

This reminds me of a recent email by Linus Torvalds https://lkml.org/lkml/2017/11/21/356 because stopping the program due to something considered dangerous is debatable.

Would you still consider an exception a good idea if the minimum value for one of the function parameters is increased in a later version of libsodium? Personally I'd prefer E_ERROR/E_WARNING.

- Chris


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
2017-11-28 15:27 GMT+01:00 Christian Schneider <[email protected]>:

> Am 28.11.2017 um 14:17 schrieb Sebastian Bergmann <[email protected]>:
> > Am 28.11.2017 um 13:56 schrieb Frank Denis:
> >> Commit: c05cbd1e775fa69ed9939796a908390f2bfb4459
> >> Author: Frank Denis <[email protected]> Tue, 28 Nov 2017
> 13:56:11 +0100
> >> Parents: c219991c77e4c68f7d62963e18a1da778de0bbe0
> >> Branches: PHP-7.2
> >>
> >> Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=
> c05cbd1e775fa69ed9939796a908390f2bfb4459
> >>
> >> Log:
> >> ext/sodium: throw exceptions instead of errors
> >
> > I am very much in favor of this change. However, making such a change in
> > PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
>
> I'm wary of turning errors into exceptions because code which was
> returning a result with libsodium before (either PECL or PHP 7.2.0) - even
> though it was generating an E_WARNING/E_ERROR - now suddenly aborts with an
> exception.
>
> Even if it was put into 7.2.0 it would break code written using the PECL
> module.
> (Side-note: I see that this case was already upgrade from E_WARNING to
> E_ERROR from PECL to PHP 7.2.0. But the program keeps running with E_ERROR
> so that's IMHO an acceptable change)
>
> This reminds me of a recent email by Linus Torvalds
> https://lkml.org/lkml/2017/11/21/356 because stopping the program due to
> something considered dangerous is debatable.
>
> Would you still consider an exception a good idea if the minimum value for
> one of the function parameters is increased in a later version of
> libsodium? Personally I'd prefer E_ERROR/E_WARNING.


I definitely want an exception there instead of an error. For a minimum
_recommendation_ a warning would be fine, but why do you prefer an E_ERROR
over an exception?

Regards, Niklas
Le 28/11/2017 à 15:27, Christian Schneider a écrit :
>>
>> I am very much in favor of this change. However, making such a change in
>> PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
>
> I'm wary of turning errors into exceptions because code which was returning a result with libsodium before (either PECL or PHP 7.2.0) - even though it was generating an E_WARNING/E_ERROR - now suddenly aborts with an exception.

I agree this change is quite bad...

> Would you still consider an exception a good idea if the minimum value for one of the function parameters is increased in a later version of libsodium?

I think a simple warning + value increased to a sane value should be
enough (could be done in PHP side, of course better if done in the library)

Remi.
Am 28.11.2017 um 16:44 schrieb Niklas Keller <[email protected]>:
> 2017-11-28 15:27 GMT+01:00 Christian Schneider <[email protected]>:
>> Would you still consider an exception a good idea if the minimum value for one of the function parameters is increased in a later version of libsodium? Personally I'd prefer E_ERROR/E_WARNING.
>
> I definitely want an exception there instead of an error. For a minimum _recommendation_ a warning would be fine, but why do you prefer an E_ERROR over an exception?

My bad. I was under the (false) assumption that PHP 7.2.0 generates an E_ERROR (and that an E_ERROR would continue executing the program) due to the diff
http://git.php.net/?p=php-src.git;a=commitdiff;h=c05cbd1e775fa69ed9939796a908390f2bfb4459
but 7.2.0RC6 generates an E_WARNING.

So forget what I said about E_ERROR.
The real question remains: Is it considered a recommendation or a requirement? The libsodium documentation says it is a requirement but the PECL/PHP7.2.0 treated it as a recommendation.

If we want to value BC then it should be kept a recommendation (i.e. E_WARNING) for future release but if we switch to the (libsodium-suggested) interpretation of treating it as a requirement then I agree that it'd be better to do this in 7.2.0 when libsodium is introduced into PHP core.

So you convinced me that we should convert it but in that case I'd do it for 7.2.0 even though we are in the final RC.

- Chris


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Tue, Nov 28, 2017 at 1:17 PM, Sebastian Bergmann <[email protected]>
wrote:

> Am 28.11.2017 um 13:56 schrieb Frank Denis:
> > Commit: c05cbd1e775fa69ed9939796a908390f2bfb4459
> > Author: Frank Denis <[email protected]> Tue, 28 Nov 2017
> 13:56:11 +0100
> > Parents: c219991c77e4c68f7d62963e18a1da778de0bbe0
> > Branches: PHP-7.2
> >
> > Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=
> c05cbd1e775fa69ed9939796a908390f2bfb4459
> >
> > Log:
> > ext/sodium: throw exceptions instead of errors
>
> I am very much in favor of this change. However, making such a change in
> PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
>
>
I agree that this is a BC break in bug fixing release and should be
reverted from PHP-7.2 IMO and kept in master only (whatever the change is -
error or exception...)

CC'd Frank...

Cheers

Jakub
Context: This commit switches some E_ERRORs introduced in a previous commit
to throw exceptions, consistently with the rest of the extension.

The original change was removing warning for parameters below an
"interactive" limit and replacing them with an error below a hard minimum.

This addresses https://bugs.php.net/bug.php?id=75572 and
https://github.com/jedisct1/libsodium-php/issues/159.

Nikita

On Tue, Nov 28, 2017 at 5:36 PM, Christian Schneider <[email protected]>
wrote:

>
> Am 28.11.2017 um 16:44 schrieb Niklas Keller <[email protected]>:
> > 2017-11-28 15:27 GMT+01:00 Christian Schneider <[email protected]>:
> >> Would you still consider an exception a good idea if the minimum value
> for one of the function parameters is increased in a later version of
> libsodium? Personally I'd prefer E_ERROR/E_WARNING.
> >
> > I definitely want an exception there instead of an error. For a minimum
> _recommendation_ a warning would be fine, but why do you prefer an E_ERROR
> over an exception?
>
> My bad. I was under the (false) assumption that PHP 7.2.0 generates an
> E_ERROR (and that an E_ERROR would continue executing the program) due to
> the diff
> http://git.php.net/?p=php-src.git;a=commitdiff;h=c05cbd1e775
> fa69ed9939796a908390f2bfb4459
> but 7.2.0RC6 generates an E_WARNING.
>
> So forget what I said about E_ERROR.
> The real question remains: Is it considered a recommendation or a
> requirement? The libsodium documentation says it is a requirement but the
> PECL/PHP7.2.0 treated it as a recommendation.
>
> If we want to value BC then it should be kept a recommendation (i.e.
> E_WARNING) for future release but if we switch to the (libsodium-suggested)
> interpretation of treating it as a requirement then I agree that it'd be
> better to do this in 7.2.0 when libsodium is introduced into PHP core.
>
> So you convinced me that we should convert it but in that case I'd do it
> for 7.2.0 even though we are in the final RC.
>
> - Chris
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
So
https://github.com/php/php-src/commit/31d221f9c72f0d0322c84907c5d89a4464667244
landed just now to revert this change, per the request here. This leaves us
with the worst variant... so, let's step back a bit:

The main commit that landed (for 7.2.1) is
https://github.com/php/php-src/commit/c219991c77e4c68f7d62963e18a1da778de0bbe0#diff-563df88ede8d2a03e291599c46952c13.
This commit *removes* checks that certain variables are below an
interactive (read: recommended) level, in which case a warning was thrown.
This resolves bug #75572. These checks were replaced with checks against
minimum values, in which case an E_ERROR is thrown. Note that these
minimums are *not* recommendations. They are hard limits imposed by the
used algorithms. The libsodium functions will *already* fail if you chose
parameters below the limits (see e.g.
https://github.com/jedisct1/libsodium/blob/d49d7e8d4f4dd8df593beb9e715e7bc87bc74108/src/libsodium/crypto_pwhash/argon2/pwhash_argon2i.c#L160).
The only thing that happened here is that these errors now get a nice,
explicit error message, instead of a generic error.

The second commit is
https://github.com/php/php-src/commit/c05cbd1e775fa69ed9939796a908390f2bfb4459,
which is what started this thread. This commit changes the E_ERRORs
introduced in the previous commit (not part of any release) to be
SodiumExceptions, because that's what ext/sodium does everywhere else.

Now
https://github.com/php/php-src/commit/31d221f9c72f0d0322c84907c5d89a4464667244
landed, which reverts that change again, based on the request here.

To summarize:

Previously: <Min: Generic SodiumException, <Interactive: Warning.
First commit: <Min: Specific E_ERROR, <Interactive: OK.
Second commit: <Min: Specific SodiumException, <Interactive: OK.
Third commit: <Min: Specific E_ERROR, <Interactive: OK.

In summary, the first two commits combined were not a BC break -- they only
removed a warning and made an error message nicer. After the third commit
we now *do* have a BC break, because what was previously a SodiumException
is now an E_ERROR.

I hope we can restore some sanity to this world by reverting that revert...

Frank: Please correct me if my understand of what's going on here is not
correct.

Regards,
Nikita

On Tue, Nov 28, 2017 at 6:02 PM, Jakub Zelenka <[email protected]> wrote:

> On Tue, Nov 28, 2017 at 1:17 PM, Sebastian Bergmann <[email protected]>
> wrote:
>
> > Am 28.11.2017 um 13:56 schrieb Frank Denis:
> > > Commit: c05cbd1e775fa69ed9939796a908390f2bfb4459
> > > Author: Frank Denis <[email protected]> Tue, 28 Nov 2017
> > 13:56:11 +0100
> > > Parents: c219991c77e4c68f7d62963e18a1da778de0bbe0
> > > Branches: PHP-7.2
> > >
> > > Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=
> > c05cbd1e775fa69ed9939796a908390f2bfb4459
> > >
> > > Log:
> > > ext/sodium: throw exceptions instead of errors
> >
> > I am very much in favor of this change. However, making such a change in
> > PHP 7.2.1 is a BC break. Any chance to get that into PHP 7.2.0?
> >
> >
> I agree that this is a BC break in bug fixing release and should be
> reverted from PHP-7.2 IMO and kept in master only (whatever the change is -
> error or exception...)
>
> CC'd Frank...
>
> Cheers
>
> Jakub
>
2017-11-29 0:03 GMT+01:00 Nikita Popov <[email protected]>:
> In summary, the first two commits combined were not a BC break -- they only
> removed a warning and made an error message nicer. After the third commit
> we now *do* have a BC break, because what was previously a SodiumException
> is now an E_ERROR.
>
> I hope we can restore some sanity to this world by reverting that revert...

All in all, E_ERROR should *never* be used by an extension, we should
revert back to using an Exception and re-tag the 7.2.0 release asap


--
regards,

Kalle Sommer Nielsen
kalle@php.net

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 29.11.2017 at 00:03, Nikita Popov wrote:

> Previously: <Min: Generic SodiumException, <Interactive: Warning.> First commit: <Min: Specific E_ERROR, <Interactive: OK.> Second
commit: <Min: Specific SodiumException, <Interactive: OK.> Third commit:
<Min: Specific E_ERROR, <Interactive: OK.> > In summary, the first two
commits combined were not a BC break -- they only> removed a warning and
made an error message nicer. After the third commit> we now *do* have a
BC break, because what was previously a SodiumException> is now an
E_ERROR.> > I hope we can restore some sanity to this world by reverting
that revert...
+1

(thanks Nikita)

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
To summarize current situation

libsodium v2.0.9 pecl extension use E_WARNING

sodium extension in php-7.2.0 (tag) use E_WARNING
(as in previous RCs)

sodium extension in PHP-7.2 (branch) use E_WARNING
(all changes have been reverted)

Seems consistent.

For 7.2.0, I don't think we have to change this, especially as there is
no real consensus about the right thing to do.



Remi


P.S. delay before 7.2.1RC1 is very short (1 or 2 weeks)


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login