Hash::Util::FieldHash: bad use of MAGIC

[leonerd]

While reading though some existing uses of PERL_MAGIC_ext to consider conversion into Hooks, I found that what FieldHash.xs does is not good. It uses ext magic with no vtable at all, instead relying on an unlikely value of 0x4944 stored in the mg_private field in the hope of uniquely identifying its own tagging.

Really, this should be done with a unique (static const) vtable, like other modules do.

[leonerd]

Additionally, it has a possible memory leak in HUF_fix_objects, when a new ID SV is assigned to an existing magic structure, where it forgets to tidy up the old one.

That said, that entire blob of code is doing very questionably-dodgy things anyway. It appears the code is just used at CLONE time for the entire interpreter, to fix up ID numbers assigned to existing objects. But this is exactly what the proper clone behaviour in magic or hooks is for anyway. There's really no need for this structure; it can be done in much better ways.

[Leont]

While reading though some existing uses of PERL_MAGIC_ext to consider conversion into Hooks, I found that what FieldHash.xs does is not good. It uses ext magic with no vtable at all, instead relying on an unlikely value of 0x4944 stored in the mg_private field in the hope of uniquely identifying its own tagging.

Really, this should be done with a unique (static const) vtable, like other modules do.

I'd call this old-fashioned instead of bad. This used to be something of an idiom (especially before 5.14 introduced mg_findext. But yeah using vtables is much better.

Additionally, it has a possible memory leak in HUF_fix_objects, when a new ID SV is assigned to an existing magic structure, where it forgets to tidy up the old one.

That said, that entire blob of code is doing very questionably-dodgy things anyway. It appears the code is just used at CLONE time for the entire interpreter, to fix up ID numbers assigned to existing objects. But this is exactly what the proper clone behaviour in magic or hooks is for anyway. There's really no need for this structure; it can be done in much better ways.

Ouch. Yeah

Then again everything about how this module is using magic is messy. The whole uvar on hashes thing was specifically added to core for this module, and yet it still sucks to the point of being barely usable (mainly because it doesn't offer a way to add state to the callbacks, so they need a second magic lookup for state. The quality control when this was added in 5.10 was not where it should have been.