Very theoretical y_hooks bug

[Y-Less]

I believe there is an issue with y_hooks, but as of yet I've not even managed to construct a synthetic test showing the problem. I'm also not going to try fixing it because the combination of events required to trigger it is insanely unlikely and the fix will slow all y_hooks initialisation down considerably. If anyone ever hits this bug - work around it, doing so is very easy! To document the bug, however, will require a crash-course in y_hooks internals.

y_hooks obviously works by rewriting functions and the AMX header to point to new callback versions. This code:

hook OnPlayerConnect(playerid)
{
}

Becomes (through y_unique):

@yH_OnPlayerConnect@014(playerid)
{
}

Or something similar. The @yH_ prefix in the compiled AMX header is exactly 4 bytes, or one cell, big. This allows us to use the prefix scanner functions to very quickly find hooks just by comparing cells instead of strings. The @014 suffix is (hopefully) a unique number to stop multiple hooks of the same function colliding with each other by having exactly the same name and causing a compiler error. This means that every hook has 8 extra charaters added to their name by y_hooks and that introduced a new problem - hooking a function with an already long name (anything over 23 characters) would push the final function name over the compiler limit of 31 characters and cause a warning. Hence replacements were born:

DEFINE_HOOK_REPLACEMENT(CP, Checkpoint);

hook OnPlayerEnterCP(playerid)
{
}

That will compile as (the number will vary):

@yH_OnPlayerEnterCP@014(playerid)
{
}

Which at run-time y_hooks will detect as normal and associate with OnPlayerEnterCheckpoint. The replacements are a little bit clever and assume CamelCase, so a replacement of Rep -> Replacement will NOT match Republic because the letter following Rep is not upper-case and so is not the start of a new word. The fact that no-one has ever noticed this behaviour leads me to believe it is working quite well. The effect is cumulative, so DynCP -> DynamicCheckpoint goes from 4 to 17 characters, a delta of 13, greater than our limit of 8, note that some callbacks may end up too long again after these replacements, so y_hooks actually re-shortens some.

Anyway, in order to rewrite the hook as the correct real callback, it needs to have space in the AMX header strings table to fit in the original callback name. If that string doesn't exist already, we need to ensure that there is space to write the new one. The strings header is packed so that each callback name is followed by NULL then immediately by the next function name.

If the original callback is ALSO in the AMX (i.e. you have a normal public version of the function along-side hooks) this is easy - we just reuse the existing string and be done with it.

If the replacement string is less than nine characters (i.e. <= 8) longer than the shortened version then we are also good, since we know we have eight extra characters to play with in the string header from all the extra information attached to the hook function name. So CP is 2 characters, Checkpoint is 10 characters - we are good.

If there is more than one hook of a callback, we are probably good, since the strings table is sorted, so two very similar callbacks will be adjacent. The numbers are at the end of the callback name, so everything before them is the same and everyone is happy. We can just clobber two consecutive strings in the header with the new function name:

@yH_OnPlayerEnterCP@014(playerid)
{
}

@yH_OnPlayerEnterCP@015(playerid)
{
}

If there is no public function and only one hook with a very large replacement (or multiple replacements) you can end up with the situation where there is not enough space in the strings table to write the expanded function name without clobbering unrelated strings and breaking random code. y_hooks can detect this situation and will issue a fatal error at run-time.

The bug comes when you somehow manage to squeeze an unrelated public in between two matching hooks in the strings table, and the first of the hook functions is not long enough to hold the final callback name on its own. For example:

DEFINE_HOOK_REPLACEMENT(CP, Checkpoint)
DEFINE_HOOK_REPLACEMENT(Dyn, Dynamic)

#include <YSI\y_hooks>
hook OnCreateDynCP(cpid)
{
}

hook OnCreateDynamicCP(cpid)
{
}

#include <YSI\y_hooks>
hook OnCreateDynObject(areaid)
{
}

This will compile as:

@yH_OnCreateDynCP@014(cpid)
{
}

@yH_OnCreateDynamicCP@014(cpid)
{
}

@yH_OnCreateDynObject@015(areaid)
{
}

Sorted lexicographically, those strings appear in this order:

@yH_OnCreateDynCP@014
@yH_OnCreateDynObject@015
@yH_OnCreateDynamicCP@014

We have managed to create two hooks that point to the same final callback (OnCreateDynamicCheckpoint), but that are separated in the string table by an unrelated function, and where the first one is only 21 characters long, but the target function name is 25 characters long. Because y_hooks currently assumes that multiple hooks for a single callback are always sequential (even though it does a full scan), it just adds all the existing lengths together and takes that final number as the number of bytes available to it for rewriting strings in the header. In this case it will see two related hooks, think it has 21+25 = 46 bytes to work with, and clobber the interloping callback, probably giving you something like:

OnCreateDynamicCheckpoint_OnCreateDynObject@015
@yH_OnCreateDynamicCP@014

Or (depending on which callback was seen first):

OnCreateDynamicCheckpointreateDynamicObject
@yH_OnCreateDynamicCP@014

This is bad! The fix is to shuffle the strings so that all the available space is at one point in the header. Maybe even shuffle further so that extra space from unrelated callbacks can be used for those that are currently detected as unwritable. This will take a lot of effort, and I didn't want to do it when I started writing this, but the realisation that the same fix can be used for another known problem is intriguing.

[Y-Less]

Buried inside that wall of text is the only mention I ever made of an actually useful y_hooks feature:

Replacements are a little bit clever and assume CamelCase, so a replacement of Rep -> Replacement will NOT match Republic because the letter following Rep is not upper-case and so is not the start of a new word. Thus the match is not matching a whole word. The only characters assumed to continue a word are a-z, so _ and @ will also end a replacement (but using @ in a hooked callback will just totally break - don't do that).

[Y-Less]

OK, it seems someone hit this...