Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling

[Tim Düsterhus]

Hi

On 9/8/22 19:06, Nicolas Grekas wrote:

 From what I understand, there are two things in the RFC:

1. a proposal to wrap any throwables thrown during unserialization
inside an UnserializationFailedException


Correct.


2. a discussion to figure out a way to turn these notices+warnings into
exceptions.


Partly correct:

The RFC primarily proposes unifying the existing non-Exception errors 
(which are currently split between E_WARNING and E_NOTICE with no 
apparent pattern). To *what* exactly (E_WARNING or some Exception) they 
are unified is up for discussion/vote.



About 1., I read that you think "this is not considered an issue", but to
me, changing the kind of exceptions that a piece of code might throw is a
non negligible BC break. There needs to be serious justification for it. I
tried looking for one, but I'm not convinced there is one: replacing the
existing catch(Throwable) by catch(UnserializationFailedException) won't
provide much added value to userland, if any. And "reliably include more
than just the call unserialize() in the try" is not worth the BC break IMHO.


unserialize() is a generic function that will also call arbitrary 
callbacks. You already have no guarantees whatsoever about the kind of 
exception that is thrown from it. I am unable to think of a situation 
where the input data is well-defined enough to reliably throw a specific 
type of exception, but not well-defined enough to successfully unserialize.


So on the contrary wrapping any Exceptions with a well-defined Exception 
allows you to rely on a specific and stable Exception to catch in the 
first place.


As you specifically mention catch(Throwable), I'd like to point out that 
this will continue to work, because UnserializationFailedException 
implements Throwable.



About 2., unserialize() accepts a second argument to give it options. Did
you consider adding a 'throw_on_error' option to opt-in into the behavior
you're looking for? I think that would be a nice replacement for the
current logics based on set_error_handler(). If we want to make PHP 9 throw
by default, we could then decide to deprecate *not* passing this option.


I did not consider this when writing the RFC. I now considered it, and I 
do not believe adding a flag to control this a good thing.


1. "No one" is going to set that flag, because it requires additional 
effort. I strongly believe that the easiest solution should also the 
correct solution for the majority of use cases. The flag fails this 
"test", because the correct solution should be "don't fail silently".


2. If you actually want to set that option in e.g. a library, then you 
break compatibility with older PHP versions for no real gain. If you go 
all the way and remember to add that extra flag, then you can also write 
an unserialize wrapper that does the set_error_handler dance I've shown 
in the introduction of the RFC. Similar effort to adding the flag, but 
better compatibility.


3. It does literally nothing for users that use a throwing error 
handler, which to my understanding includes the vast majority of 
framework code.


4. Even for users that do not use a throwing error handler, omitting the 
option literally does nothing, because unserialize() might already throw 
depending on what the unserialize handlers of unserialized objects do.



Lastly I'd like to add a 3. to your proposal, because there is one more
thing that makes unserialization annoying: the unserialize_callback_func

ini setting. It would be great to be able to pass a 'callback_func' option
to unserialize to provide it, instead of calling ini_set() as we have to
quite often right now.

Would that make sense to you?



TIL about that ini setting. Can you clarify where this callback comes in 
helpful? What can it do for you what your autoloader can't do? I've 
attempted to search GitHub to find out about the use cases, but almost 
all of the results are copies of php-src that match the .phpt tests.


However as of now I do not believe that it is appropriate to include in 
my RFC, because it is only indirectly related to error handling. I'd 
like to keep the RFC focused. This makes it easier for readers to 
understand the proposal, allowing voters to make an educated vote and 
serving as longer-term documentation if the vote passes.


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling

[Nicolas Grekas]

Hi Tim,

Thanks for the RFC.

I've now written up an RFC as a follow-up for the "What type of
> Exception to use for unserialize() failure?" thread [1]:
>
> 
>
> RFC: Improve unserialize() error handling
> https://wiki.php.net/rfc/improve_unserialize_error_handling
>
> Proof of concept implementation is in:
>
> https://github.com/php/php-src/pull/9425
>
> Discussion period for that RFC is officially opened up.
>
> 
>
> The primary point of discussion in the previous mailing list thread and
> in the PR comments is whether unserialize() should continue to emit
> E_WARNING or whether that should consistently be changed to an
> Exception. As of now I plan to explicitly vote on this and the RFC
> contains some opinions on that matter.
>

>From what I understand, there are two things in the RFC:

   1. a proposal to wrap any throwables thrown during unserialization
   inside an UnserializationFailedException
   2. a discussion to figure out a way to turn these notices+warnings into
   exceptions.

About 1., I read that you think "this is not considered an issue", but to
me, changing the kind of exceptions that a piece of code might throw is a
non negligible BC break. There needs to be serious justification for it. I
tried looking for one, but I'm not convinced there is one: replacing the
existing catch(Throwable) by catch(UnserializationFailedException) won't
provide much added value to userland, if any. And "reliably include more
than just the call unserialize() in the try" is not worth the BC break IMHO.

About 2., unserialize() accepts a second argument to give it options. Did
you consider adding a 'throw_on_error' option to opt-in into the behavior
you're looking for? I think that would be a nice replacement for the
current logics based on set_error_handler(). If we want to make PHP 9 throw
by default, we could then decide to deprecate *not* passing this option.

Lastly I'd like to add a 3. to your proposal, because there is one more
thing that makes unserialization annoying: the unserialize_callback_func

ini setting. It would be great to be able to pass a 'callback_func' option
to unserialize to provide it, instead of calling ini_set() as we have to
quite often right now.

Would that make sense to you?

Kind regards,
Nicolas

Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling

[Larry Garfield]

On Thu, Sep 8, 2022, at 11:18 AM, Tim Düsterhus wrote:
> Hi
>
> On 9/7/22 23:44, Larry Garfield wrote:
>> Either I guess?  Honestly we should decide that in advance on the list. :-)  
>> E_WARNING+Exception in 9 is what I'd probably favor, with "Exception now" as 
>> a second choice.
>> 
>
> I'm a new-ish contributor here in internals, so I don't know how things 
> were done in the past for similar situations/issues.
>
> I'm not sure, though if it makes sense to already decide on something 
> for PHP 9. If it's not baked into code shortly after the vote finishes, 
> then people might forget that "there's something that still needs to be 
> done". For a deprecation one can at least go through all the 
> deprecations once PHP 9 opens, as a deprecation effectively is defined 
> to be a removal in the next major. For a warning this is less obvious.
>
> Personally my first choice would be "Straight to Exception", so I might 
> not be the best person to decide on that :-)
>
> Best regards
> Tim Düsterhus

We've done this kind of two-step thing before; a lot of PHP 8 changes were 
voted on well in advance of 8.0's release.  A "Warning now, Exception in 9" 
vote would not be unprecedented.

--Larry Garfield

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling

[Tim Düsterhus]

Hi

On 9/7/22 23:44, Larry Garfield wrote:

Either I guess?  Honestly we should decide that in advance on the list. :-)  
E_WARNING+Exception in 9 is what I'd probably favor, with "Exception now" as a 
second choice.



I'm a new-ish contributor here in internals, so I don't know how things 
were done in the past for similar situations/issues.


I'm not sure, though if it makes sense to already decide on something 
for PHP 9. If it's not baked into code shortly after the vote finishes, 
then people might forget that "there's something that still needs to be 
done". For a deprecation one can at least go through all the 
deprecations once PHP 9 opens, as a deprecation effectively is defined 
to be a removal in the next major. For a warning this is less obvious.


Personally my first choice would be "Straight to Exception", so I might 
not be the best person to decide on that :-)


Best regards
Tim Düsterhus

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Re: Increase maximum size of an uploaded file to 50Mbyte

[Christoph M. Becker]

On 08.09.2022 at 13:01, Andreas Heigl wrote:

> On 08.09.22 12:35, Jakub Zelenka wrote:
>
>> On Wed, Sep 7, 2022 at 3:28 PM Christoph M. Becker 
>> wrote:
>>
>>> On 07.09.2022 at 15:57, Misha wrote:
>>>
 We spend a lot of time to increase limits for uploads file in PHP.
 Can we
 increase it in php.ini?

 Current value is 2Mb. Its so small value, when photo image can take 8Mb
>>> on
 iPhone X.
 We should increase it to 50Mb, because DevOps engineers do useless work
 trying to change it.

 I have prepared PR for it https://github.com/php/php-src/pull/9315
>>>
>>> I'm not against increasing the sizes, but 50MB might be too much.
>>>
>>> Anyway, only changing the php.ini files doesn't make sense in my
>>> opinion, since they're probably only used on Windows, and they should
>>> reflect the actual default values[1].
>>
>>   It's true that those particular ini files are not directly used on
>> Linux
>> but distros often base their changes on them and the distro provided ones
>> are actually used. So it means that main defaults in main.c are most
>> likely
>> not used. Although I agree they should be changed too so it is
>> consistent.
>
> No matter which value we preset, it will most certainly not be adequate.
>
> So people *will* have to set it according to their needs. And if they
> didn't so far, then they are happy with the current setting.
>
> Don't get me wrong: I'm not against changing that value.
>
> But is it really an issue that needs solving? It's a default value of a
> configuration file. When I'm unhappy with the default, I change it to a
> more suitable value. When I'm not able to do that... perhaps I should
> leave my fingers not only from the config file...

It might be worth to point out that we're using these defaults since PHP
4.0 (at least), so reconsidering sensible defaults might be appropriate.

--
Christoph M. Becker

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php

Re: [PHP-DEV] Increase maximum size of an uploaded file to 50Mbyte

[Kamil Tekiela]

I have it set to 32MB and nobody has complained yet. I think it might be a
sweet spot between security and usability out of the box.

Re: [PHP-DEV] Increase maximum size of an uploaded file to 50Mbyte

[Jakub Zelenka]

On Wed, Sep 7, 2022 at 2:58 PM Misha  wrote:

> Hello everyone,
>
> We spend a lot of time to increase limits for uploads file in PHP. Can we
> increase it in php.ini?
>
> Current value is 2Mb. Its so small value, when photo image can take 8Mb on
> iPhone X.
> We should increase it to 50Mb, because DevOps engineers do useless work
> trying to change it.
>
>
I think the problem is that too high value can potentially result in DoS if
you have for example some API that might be sensitive to it. I think this
should be really handled on web server though and Apache httpd [1] as well
as nginx [2] have the default limit set to 1MB. The only problem is that
Apache introduced that limit default quite recently (2.4.53) so there are
likely still lots of users where this value matters more if they don't
tweak defaults. I guess it might be wise to do much smaller increase and
start maybe somewher closer to 8MB or maybe even wait a little bit longer
till most users have safe defaults on web server.

[1] https://httpd.apache.org/docs/2.4/mod/core.html#limitrequestbody
[2]
http://nginx.org/en/docs/http/ngx_http_core_module.html#client_max_body_size

Regards

Jakub

Re: [PHP-DEV] Re: Increase maximum size of an uploaded file to 50Mbyte

[Andreas Heigl]

Hey all.

On 08.09.22 12:35, Jakub Zelenka wrote:

On Wed, Sep 7, 2022 at 3:28 PM Christoph M. Becker 
wrote:


On 07.09.2022 at 15:57, Misha wrote:


We spend a lot of time to increase limits for uploads file in PHP. Can we
increase it in php.ini?

Current value is 2Mb. Its so small value, when photo image can take 8Mb

on

iPhone X.
We should increase it to 50Mb, because DevOps engineers do useless work
trying to change it.

I have prepared PR for it https://github.com/php/php-src/pull/9315


I'm not against increasing the sizes, but 50MB might be too much.

Anyway, only changing the php.ini files doesn't make sense in my
opinion, since they're probably only used on Windows, and they should
reflect the actual default values[1].



  It's true that those particular ini files are not directly used on Linux
but distros often base their changes on them and the distro provided ones
are actually used. So it means that main defaults in main.c are most likely
not used. Although I agree they should be changed too so it is consistent.


No matter which value we preset, it will most certainly not be adequate.

So people *will* have to set it according to their needs. And if they 
didn't so far, then they are happy with the current setting.


Don't get me wrong: I'm not against changing that value.

But is it really an issue that needs solving? It's a default value of a 
configuration file. When I'm unhappy with the default, I change it to a 
more suitable value. When I'm not able to do that... perhaps I should 
leave my fingers not only from the config file...


My 0.02€

Cheers

Andreas
--
  ,,,
 (o o)
+-ooO-(_)-Ooo-+
| Andreas Heigl   |
| mailto:andr...@heigl.org  N 50°22'59.5" E 08°23'58" |
| https://andreas.heigl.org   |
+-+
| https://hei.gl/appointmentwithandreas   |
+-+


OpenPGP_0xA8D5437ECE724FE5.asc
Description: OpenPGP public key


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PHP-DEV] Re: Increase maximum size of an uploaded file to 50Mbyte

[Jakub Zelenka]

On Wed, Sep 7, 2022 at 3:28 PM Christoph M. Becker 
wrote:

> On 07.09.2022 at 15:57, Misha wrote:
>
> > We spend a lot of time to increase limits for uploads file in PHP. Can we
> > increase it in php.ini?
> >
> > Current value is 2Mb. Its so small value, when photo image can take 8Mb
> on
> > iPhone X.
> > We should increase it to 50Mb, because DevOps engineers do useless work
> > trying to change it.
> >
> > I have prepared PR for it https://github.com/php/php-src/pull/9315
>
> I'm not against increasing the sizes, but 50MB might be too much.
>
> Anyway, only changing the php.ini files doesn't make sense in my
> opinion, since they're probably only used on Windows, and they should
> reflect the actual default values[1].
>
>
 It's true that those particular ini files are not directly used on Linux
but distros often base their changes on them and the distro provided ones
are actually used. So it means that main defaults in main.c are most likely
not used. Although I agree they should be changed too so it is consistent.

Regards

Jakub