Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

read_property() handler is not supposed to return NULL #8362

Merged

Conversation

tony2001
Copy link
Contributor

@tony2001 tony2001 commented Mar 2, 2021

This handler is called throughout the Zend engine code and in none of the cases the result is checked for NULL because the engine expects it to be a valid zval.
If there is no such property, the object has to return &EG(uninitialized_zval).

NULL is never expected as a result of read_property() handler,
it should return &EG(uninitialized_zval) if there is no such property in
the object
@haberman
Copy link
Member

haberman commented Mar 3, 2021

I agree this makes sense to fix. But can I ask how you encountered this? User code should never be accessing message properties directly.

@tony2001
Copy link
Contributor Author

tony2001 commented Mar 3, 2021

It's not that easy.
We have some debugging code incorporated into several million lines of code in PHP. This debugging code tries to provide as much info as possible and dumps all the objects with all their properties (I think. It's a lot of code and I'm not really familiar with it).

I believe that's actually the problem with protobuf extension - it always presumes that user should not be doing this or that. There are no safety checks, it just crashes if user does something it was not designed to do (like instantiating an internal class manually).
Unfortunately, there's no way to prevent that - users (i.e. PHP programmers in this case) sometimes do strange things, sometimes their code is used not the way it was initially designed to. And they expect to get errors, not crashes.

@haberman
Copy link
Member

haberman commented Mar 3, 2021

There are no safety checks, it just crashes if user does something it was not designed to do (like instantiating an internal class manually).

I agree we should enforce these rules whenever possible. For example there is #8002 which might have been able to prevent this.

@haberman haberman merged commit 4baed79 into protocolbuffers:master Mar 3, 2021
This was referenced Mar 7, 2021
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants