FLHook SVN discussion
-
I create this topic so that one can post things he noticed in the SVN version.
While catching up with current development (last time I checked was about a year ago) I noticed some things.
While things like```
// Check to see if the hook IDS limit has been reached
static uint solar_ids = 526000;
if (++solar_ids > 526999)// fill struct
__asm
{
lea ecx, packetSolar
mov eax, address1
call eax
push solar
lea ecx, packetSolar
push ecx
mov eax, address2
call eax
}
of SpawnSolar is breaking the stack. You are missing
add esp, 8I also would suggest surrounding it with pushad and popad, since in case you add more code which leads to using registers the subroutines change, this could also lead to strange behaviour. Also you notice immediately if the stack is broken, since you get a crash. The corrected version thus looks like:// fill struct
__asm
{
pushad
lea ecx, packetSolar
mov eax, address1
call eax
push solar
lea ecx, packetSolar
push ecx
mov eax, address2
call eax
add esp, 8
popad
}It took me around an hour to get suspicious, as my code started to behave oddly. So please, while this is really cool stuff (bases have visible loadouts now etc, yay!), do a double check or tripple for at least the asm stuff. I know I am a perfectionist and thus easily annoyed by such stuff, but I think this is really worth it ;) If I find more stuff I will post it. -
Hehe, no need to be
-
Just wanted to mention that this stack corruption is also in the DefenseModule
And in swprintf routines its better to call _countof instead of sizeof. sizeof returns the wrong value for wide character strings and the possibility of memory corruption might occur too.
-
I think he just forgot. The calls are mostly correct and use wsclen stuff, but on a few occasions it is indeed sizeof, which gives double the count. Shouldn’t be that bad, though, because of the formatted texts being short in those cases (just had a glance at it).
-
Well sizeof can be used for char, for wchar its better to use _countof. wsclen is just the length of the string but not the size of the buffer that holds these strings
And this is not only for Cannons plugin, that is also in the base FlHook version and in some other plugins too
-
I am not sure I follow you. The base FLHook does not even use the overloaded call for swprintf where you also pass the buffer length. There are only a few calls which use sizeof and I agree that they are problematic.
-
Schmack, I’ve added you to the dev group. You can now patch things yourself.
Huor, you don’t have a Forge account just yet, but if you do make one I can add you too.
-
It’s in SVN, DJ.DBFuz: http://svn.the-starport.net/flhookplugin/
-
@FF Thanks, I will correct it when I find the time. Maybe I add some stuff I recently reversed, too.
-
FriendlyFire wrote:
…Huor, you don’t have a Forge account just yet, but if you do make one I can add you too.Now i remember i already have an account - dunno since months, but it does not let me in. Yesterday i tried again but it refuses - even with a new password. So there is something wrong. Maybe someone might have a look into this?!
Thanks in advance.
-
I’ve checked and I do not see a user named “huor” so I think you don’t have an account.
-
Just to come back to topic. Its something i am currently stumbled.
In some of the function calls FlHook uses a variable argument list to handle multiple arguments. However such a variable argument list must be started withva_start ()
call. So far so good, but to clean the stack it is required to have the
va_end ()
as well, which i searched for, is missing in every call. I am not sure what happens if its left, but i learned that both should be used together. Whether i am not sure if thats important - i corrected it now in my version.
-
Well, if you look at the documentation for the msvc compiler there only is stated that the pointer is set to NULL. Nowhere is explicitly written that you have to call it. The example with void testit(…) doesn’t. The one after that does.
If you look on www.cplusplus.com they do state this. Whether this causes errors - no idea. Doesn’t seem to, though. With gcc etc this might be different.
-
Just want to mention that the last added revision from w0dka (236) seems to miss the pushad and popad commands within the asm code [in dynamic_missions/Hooks.cpp]
Schmack already pointed that out in the base plugin code where it was corrected already. Just thought to mention it.
-
Yeah, I see what you mean. I’d call it risky, since the registers after the calls from the asm code could contain different values than the msvc compiler assumes while compiling, leading to unexpected behaviour.
-
Any function you call will clean up and preserve registers itself. A function that does not do that is generally buggy/faulty.
Thats why you dont need pushads/pops around “call xy”.As regards __asm blocks, msdn has this to say:
When using __asm to write assembly language in C/C++ functions, you don’t need to preserve the EAX, EBX, ECX, EDX, ESI, or EDI registers.
You should preserve other registers you use (such as DS, SS, SP, BP, and flags registers) for the scope of the __asm block. You should preserve the ESP and EBP registers unless you have some reason to change them (to switch stacks, for example).
