Reimagined: Correctly handle lengths of serialized strings

[abdullah-kasim]

Hello, this PR is an alternative to the PR in #39 and should fix #33 .

Regex isn't too great for this purpose, as it has a tendency to over-match. Hence, this PR offers an alternative solution.

This PR uses a mini-tokenizer/parser to detect instances of PHP String, then do search-replace in the constraints of the token, then go back up the stack to recalculate the new byte size.

Special care has been taken care so that we didn't include MySQL's sqldump escaping.

It should technically work with mydumper as well - upon inspecting a mydumper dump, the escaping is very much the same.

There's a few errors with the tests in the PR #39 which I've fixed, as well as added new tests to handle some edge cases with excessive escaping, as well as with very plausible scenario of someone using the string combination of \"; i.e. in WP posts.

If approved, I'll re-point the branch to trunk and merge it from there.

[abdullah-kasim]

Rather hard to do, but it's possible that the regex matches inside say, a JSON, and it'd have some sort of multi-layer escaping. Someone might also accidentally use the same combination of string i.e. s:123:", as maybe he's writing a programming tutorial. So we'd accidentally match these, and we should match as little of these as possible, because it'll affect the parser's stability.

[mjangda]

Should we just remove the TODO and leave the comment as a note about the edge case?

[abdullah-kasim]

Sure, that makes sense.

[luismulinari]

This is awesome!

Just wondering if it would be worth adding some test cases from: https://github.com/php/php-src/tree/master/ext/standard/tests/serialize

It has a pretty good set of edge cases.

[nickchomey]

Nice work! I was thinking about this problem recently and found these two packages for handling php serialized strings in Go.

• https://github.com/elliotchance/phpserialize
• https://github.com/trim21/go-phpserialize

I asked in an issue in the 2nd repo what the difference was and the dev gave a nice answer.
trim21/go-phpserialize#53

I wonder if this (finding the serialized strings, unserialize, modify, reserialize) would be a more robust way of handling this? (I brought it up in #42 but perhaps it wasn't noticed/relevant)

[luismulinari]

Also, do you think it would be worth adding some fuzzy tests?

[abdullah-kasim]

Yeah I definitely need more tests. Thanks for those suggestions. @luismulinari !

[abdullah-kasim]

Nice work! I was thinking about this problem recently and found these two packages for handling php serialized strings in Go.

• elliotchance/phpserialize
• trim21/go-phpserialize

I asked in an issue in the 2nd repo what the difference was and the dev gave a nice answer. trim21/go-phpserialize#53

I wonder if this (finding the serialized strings, unserialize, modify, reserialize) would be a more robust way of handling this? (I brought it up in #42 but perhaps it wasn't noticed/relevant)

@nickchomey I did notice your post about these libraries, and yeah I agree I should add some comments about it :) . The reason why I didn't utilize these libraries, is that it came down to simplification and speed.

Neither of these two libraries are designed to parse something from a sqldump, so we first need to parse the sqldump so that these serialization library can do its work.

Which means, either we write a SQL parser, or use something existing, like vitess-sqlparser. But we can't simply use a SQL parser, as per this reason.

Why

xwb1989/sqlparser is famous sql parser in Go.
But it cannot parse some query (like offset or bulk insert...) because it customizes vitess's sql parser.

Also, some libraries use from vitess sql parser directly. But vitess's sql parser only partial supports DDL parsing.

We want to perfectly support parsing for SQL and DDL.
Therefore we use vitess sql parser directly and also use TiDB parser for DDL parsing.

And so we're hitting a complexity point here - if we use a pre-existing sql parser, we're at the whims of that parser library. Certain lines in the sqldump might not work. Maybe it doesn't support certain mysql syntax, maybe we have some nasty edge-case in the sqldump, we'd need to check for mydumper compatibility as well. Admittedly I only did some thought experiments but didn't test-drive it for real. And I think it'll be slower than this PR.

And that's how I decided to just write my own parser. Took at most a day for the core code.

[nickchomey]

Fair enough! Hopefully this can get merged soon enough!

[abdullah-kasim]

This is awesome!

Just wondering if it would be worth adding some test cases from: php/php-src@master/ext/standard/tests/serialize

It has a pretty good set of edge cases.

@luismulinari There's a bit too many test cases in there, and there's a lot that is unrelated to string serialization. Anything specific that you wish to point out?

[abdullah-kasim]

Also, do you think it would be worth adding some fuzzy tests?

@luismulinari

Probably not worth it - rather difficult to fuzz due to escaping and all so the fuzzing needs to have escaping logic itself 😅 , and the function pretty obviously short-circuits to just returning the line if it fails to parse it. Not enough branching for it, I suppose.

At least that's my take on it right now.

[abdullah-kasim]

Benchmarks in case they are a concern:

cpu: Apple M2 Max
BenchmarkFix-12                          1864381               641.4 ns/op
BenchmarkNoReplaceOld-12                149462286                8.210 ns/op
BenchmarkNoReplaceNew-12                 6201714               193.3 ns/op
BenchmarkSimpleReplaceOld-12            100000000               10.71 ns/op
BenchmarkSimpleReplaceNew-12             5087581               238.7 ns/op
BenchmarkSerializedReplaceOld-12        70218040                17.00 ns/op
BenchmarkSerializedReplaceNew-12         4164714               286.2 ns/op

20x slow-down?

Real world: (2.8 GB DB dump)

[ 18:18:09 ] time cat dump.sql | ./go-search-replace-new domain-a.com domain-b.com > test-1.sql

real    0m2.093s
user    0m4.952s
sys     0m3.376s

go-search-replace on  trunk [📝] via  v1.23.2 via 🐘 v8.3.12 took 2s 
[ 18:18:14 ] time cat dump .sql | ./go-search-replace-old domain-a.com domain-b.com > test-1.sql

real    0m1.332s
user    0m1.661s
sys     0m1.818s

Not too bad I suppose...

[mjangda]

Shared some minor notes otherwise this looks good.

I did some separate performance testing as well (different size files, including some that are 20GB+, and have lots of serialized data). In most cases, this update is slower but not enough to be a concern. In one case (lots of serialized content), this was actually faster :) But I think results will vary depending on size, lines, complexity, etc.

[mjangda]

Should we just remove the TODO and leave the comment as a note about the edge case?