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

fix: replace null character when serializing #49528

Merged
merged 1 commit into from
Dec 14, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Nov 28, 2024

Summary

Serialize() creates string with null character, this causes issues when saving data in certain databases

Checklist

@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-47879-property-serialization branch from c8ec037 to 7e22304 Compare November 28, 2024 03:32
@SebastianKrupinski SebastianKrupinski self-assigned this Nov 28, 2024
@SebastianKrupinski SebastianKrupinski added the 3. to review Waiting for reviews label Nov 28, 2024
@SebastianKrupinski
Copy link
Contributor Author

/backport to stable30

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable29

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable28

@tcitworld
Copy link
Member

While this seems a much simple way of fixing the issue that what I've tried at #39084, what happens when you unserialize the values? Are the objects valid and make sense, since you loose the fact that a property is set to null?

You also need to add a repair step to remove all broken values (see preSchemaChange in file Version1032Date20230630084412.php of my own PR).

@SebastianKrupinski
Copy link
Contributor Author

While this seems a much simple way of fixing the issue that what I've tried at #39084, what happens when you unserialize the values? Are the objects valid and make sense, since you loose the fact that a property is set to null?

Yes, they properly unserialize to a php object. If you replace the null character with a space. Tested this.

You also need to add a repair step to remove all broken values (see preSchemaChange in file Version1032Date20230630084412.php of my own PR).

yes, a repair step will be needed. I just need to investigate, if we can just do one for Postgres only, MySQL seems to handle null character properly.

@tcitworld
Copy link
Member

But the property that was previously null will now be set to a space. Won't that cause any trouble?

@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Nov 28, 2024

But the property that was previously null will now be set to a space. Won't that cause any trouble?

So this is the value that was causing the issue,
O:48:"Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp":1:{s:8:" * value";s:6:"opaque";}

The part to the sting that is the issue is the " * value" the space before and after the * are a null characters, basically this "null*null". The word value is actually a property inside the object. I don't know all the in and outs of serialize but to me it looks like "null*null" is a marker for a private property.

On that note, you might be right if there is a value that is null and serialize converts it to a null character then there might be a issue. But then we are back to square 1, and there is no real way to fix this for postgres. The only other way I can think of, would be to replace the null with the word null and then replace it again on unserialize.

@tcitworld
Copy link
Member

But then we are back to square 1, and there is no real way to fix this for postgres. The only other way I can think of, would be to replace the null with the word null and then replace it again on unserialize.

The only way I've thought of is to have the column type accept null terminators, by setting it to BLOB. But data migration is a hassle.

I think your solution might be an easier workaround that we can try until some other serious issue gets reported. It just needs the cleanup step.

@SebastianKrupinski
Copy link
Contributor Author

I think your solution might be an easier workaround that we can try until some other serious issue gets reported. It just needs the cleanup step.

I agree, done and done, made the repair step actually check if the property is broken, instead of just blindly removing them all.

@SebastianKrupinski
Copy link
Contributor Author

SebastianKrupinski commented Dec 6, 2024

Done. Made it handle all object properties and run as a expensive repair step.

lib/private/Repair/RemoveBrokenProperties.php Outdated Show resolved Hide resolved
lib/private/Repair/RemoveBrokenProperties.php Outdated Show resolved Hide resolved
lib/private/Repair/RemoveBrokenProperties.php Outdated Show resolved Hide resolved
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane!

Could you add a test for the unserialization of a legacy object?

lib/private/Repair/RemoveBrokenProperties.php Show resolved Hide resolved
lib/private/Repair/RemoveBrokenProperties.php Outdated Show resolved Hide resolved
@SebastianKrupinski
Copy link
Contributor Author

Could you add a test for the unserialization of a legacy object?

Done.

Did one for the current double encoding and for the legacy single encoding.

@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-47879-property-serialization branch from 116b7e5 to c1dd8dd Compare December 13, 2024 16:46
@SebastianKrupinski SebastianKrupinski merged commit dd89911 into master Dec 14, 2024
185 of 188 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-47879-property-serialization branch December 14, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

[Bug]: Trying to share a calendar in the iOS app breaks it [Bug]: Error PHP unserialize
3 participants