[PHP-DEV] SensitiveParameterValue serialization behavior
Hi Internals!
during code review of the "Redacting parameters in back traces" RFC [1]
an issue with the proposed serialization behavior of
SensitiveParameterValue objects became apparent that was not noticed
before the RFC went into voting:
The RFC proposed that serialization was allowed, but without including
the inner value in the serialization data:
public function __serialize(): array { return []; }
As this operation is lossy, it was proposed that unserialization fails
and this is what was implemented in the PoC patch:
public function __unserialize(array $data): void {
throw new \Exception('...');
}
The decision to allow serialization was to allow existing error handlers
to work without needing to special case SensitiveParameterValue. However
it is clearly not useful, if unserialization does not work after all.
Any error during unserialization is not recoverable.
Please find the thread in the GitHub PR at:
https://github.com/php/php-src/pull/7921#discussion_r813743903
As per Ilija Tovilo's suggestion I'm looping in the Internals list as well.
I see two possible options to remediate this issue:
---
1. Disallow both serialization and unserialization.
This will make the serialization issue very obvious, but will require
adjustments to exception handlers that serialize the stack traces.
2. Allow unserialization, but poison the unserialized object and
disallow calling ->getValue() on it.
This would be closer to the original intent of the RFC, but moves the
issue just somewhere else: The object would not be usable either way.
---
What would be your preferred option? Feel free to either reply on the
list or add to the discussion on GitHub.
Thanks!
[1] https://wiki.php.net/rfc/redact_parameters_in_back_traces
Best regards
Tim Düsterhus
Developer WoltLab GmbH
--
WoltLab GmbH
Nedlitzer Str. 27B
14469 Potsdam
Tel.: +49 331 96784338
duester...@woltlab.com
www.woltlab.com
Managing director:
Marcel Werk
AG Potsdam HRB 26795 P
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
Hi On 2/24/22 09:47, Go Kudo wrote: RFC has been updated. Is this up to the required standard? https://wiki.php.net/rfc/rng_extension I've just passed my own RFC just yesterday, so I'm certainly not an expert with regard to RFC standards, however: I think the explanation in the RFC is *much* better now. It explains all the important aspects, so that one does not need to look into the implementation to understand how it is supposed to work. However it would benefit from a little more structuring. I suggest to add some additional sub-headings to the "Proposal" section. Perhaps something like this: # Proposal ## Current Issues ### Global State ### Mersenne Twister is not state of the art ## Random Engine ### MT ### xorshift ### xoshiro ### ## Randomizer I'm not sure whether all those headlines make sense, you're the expert here. I acknowledge that the previous RFC may have been difficult to discuss. If the problem has been solved, I would like to make another ML-wide announcement and wait for two weeks from there. I would advice you not to rush the vote. Take your time to make the RFC perfect. It does not help if you start the vote in 2 weeks and then the RFC is declined, because of some issues. 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] [Under Discussion] Random Extension 4.0
On 24 February 2022 09:47:39 CET, Go Kudo wrote: > >Hi > >RFC has been updated. Is this up to the required standard? >https://wiki.php.net/rfc/rng_extension > >I acknowledge that the previous RFC may have been difficult to discuss. If >the problem has been solved, I would like to make another ML-wide >announcement and wait for two weeks from there. > >I added PCG64 because according to the RNG experts, there seems to be a >mild conflict between Xorshiro256 and PCG64. Also, as mentioned in the RFC, >Rust and NumPy also implement PCG64. > >In order to verify the feasibility of PCG64, we created a PoC in C. So far, >it seems to work fine. >https://github.com/zeriyoshi/pcg64_example > >Regards, >Go Kudo Hello, Two small nits: - The "Backward Incompatible Changes" section is missing the fact that `\Random\Randomizer` will be reserved. - In the definition of `Randomizer` in the "Proposal" section, there is a typo: `function getBytes(int $legnth)` should be `$length` instead. To me, this RFC looks very good so far - I don't have a vote however. Regards, Mel -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] Undefined Variable Error Promotion
On 23/02/2022 23:03, Mark Randall wrote: On 23/02/2022 19:32, Rowan Tommins wrote I think that wording change should be part of the proposed change in the RFC. Otherwise, a lot of people simply won't know the decision to remove it has been made and will be surprised when 9.0 comes around. It is already part of the RFC within the "proposed PHP versions" section. It doesn't mention any particular wording, and says the change "may be included". I'm not sure what that means. If I vote "yes", am I writing a blank cheque for anyone to change the message to anything? More importantly, am I approving removal even if the warning message isn't changed? I'm not sure when else this decision would be made, other than as part of the RFC, so am suggesting the "may" be changed to "will", and a specific wording proposed. Or, if you think changing the message isn't necessary, "may" can be changed to "will not", and people can vote on that basis. Regards, -- Rowan Tommins [IMSoP] -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: https://www.php.net/unsub.php
Re: [PHP-DEV] [RFC] [Under Discussion] Random Extension 4.0
2022年2月21日(月) 21:44 Tim Düsterhus : > Hi > > On 2/21/22 03:57, Go Kudo wrote: > > I am sorry for the delay in replying. > > Don't worry, there was a weekend inbetween and I totally understand that > one wants to take their weekend off. > > > Thank you for the clear explanation. > > It is true that the RFC in its current form lacks explanation. I'll try > to > > fix this first. > > Sounds good. > > > Also, as I look into other languages' implementations, I see the need to > > add some RNGs such as PCG. I will update the RFC to include these. > > I suggest you avoid "feature creep" within the RFC. Additional engines > can be added easily later on if the need arises. But for now it's more > important to get a reliable basis that one can build onto. > > Having a choice of a multitude of different engine just distracts from > that goal and can be confusing for the user. With xoshiro256** there's a > very good choice that already is part of the RFC, no need to have > something else that might or might not be slightly better in some case. > > Best regards > Tim Düsterhus > Hi RFC has been updated. Is this up to the required standard? https://wiki.php.net/rfc/rng_extension I acknowledge that the previous RFC may have been difficult to discuss. If the problem has been solved, I would like to make another ML-wide announcement and wait for two weeks from there. I added PCG64 because according to the RNG experts, there seems to be a mild conflict between Xorshiro256 and PCG64. Also, as mentioned in the RFC, Rust and NumPy also implement PCG64. In order to verify the feasibility of PCG64, we created a PoC in C. So far, it seems to work fine. https://github.com/zeriyoshi/pcg64_example Regards, Go Kudo
