alliedmodders / sourcemod

SourceMod - Source Engine Scripting and Administration
http://www.sourcemod.net/
979 stars 424 forks source link

Crash from 6922 version #1833

Closed mostten closed 1 year ago

mostten commented 2 years ago

Crash on Nmrih server with Sourcemod 6922. Report:"classname missing from entity!"

mostten commented 2 years ago

Stack Trace: https://crash.limetech.org/jvdhnt4id7u4 Function KERNELBASE.dll!RaiseException + 0x58 engine.dll + 0x1fcc1a engine.dll + 0x1fca91 engine.dll + 0x1fd5a5 tier0.dll + 0x52e0 server.dll + 0x1b160d server.dll + 0x21755d server.dll + 0x2175d3 metamod.2.sdk2013.dll!SourceHook_MFHCls_SGD_LevelInit::Func(char const ,char const ,char const ,char const ,bool,bool) [metamod.cpp:83 + 0xe3] sourcemod.2.sdk2013.dll!SourceModBase::LevelInit(char const ,char const ,char const ,char const ,bool,bool) [sourcemod.cpp:436 + 0x4d] metamod.2.sdk2013.dll!__SourceHook_MFHCls_SGD_LevelInit::Func(char const ,char const ,char const ,char const ,bool,bool) [metamod.cpp:83 + 0x8c] engine.dll + 0x14fc92 engine.dll + 0x1a21d2 engine.dll + 0x1b2cec engine.dll + 0x1b2404 engine.dll + 0x200a77 engine.dll + 0x1ff9bd engine.dll + 0x21f17c dedicated.dll + 0x3e85 dedicated.dll + 0x24b6c dedicated.dll + 0x4be8 srcds.exe + 0x158e srcds.exe + 0x26fd kernel32.dll!BaseThreadInitThunk + 0x12 ntdll.dll!RtlUserThreadStart + 0x27 ntdll.dll!_RtlUserThreadStart + 0x1b

nosoop commented 2 years ago

Looks like an issue introduced with the integration of entity lump handling code that I've contributed.

Can you provide the following additional information for me to try and reproduce this?

It sounds like either there's a parse failure on the particular map you're running, a conflict with another component that manipulates the entity lump (Stripper:Source, a copy of the Entity Lump extension), or a plugin you're using or have created is removing entries from the entity lump that shouldn't be removed.

mostten commented 2 years ago

Looks like an issue introduced with the integration of entity lump handling code that I've contributed.

Can you provide the following additional information for me to try and reproduce this?

  • Which map this crashed on (if it's a custom map, please provide a link to download it)
  • SourceMod plugins, SourceMod extensions, and Metamod:Source plugins you've installed

It sounds like either there's a parse failure on the particular map you're running, a conflict with another component that manipulates the entity lump (Stripper:Source, a copy of the Entity Lump extension), or a plugin you're using or have created is removing entries from the entity lump that shouldn't be removed.

NMRiH has a Maphack system, the same as Stripper:Source, It has a way to remove entities.

mostten commented 2 years ago

Example: nmo_chinatown.txt

Text:

"pre_entities" { $remove_all{"classname" "random_spawner"} }

nosoop commented 2 years ago

@dysphie has previously tested the entity lump changes in NMRiH; I don't believe the Maphack system would conflict in this case. Giving them a shout in the event that this case was overlooked.

I'll try to reproduce the crash with the information and example files you've provided.

mostten commented 2 years ago

@dysphie has previously tested the entity lump changes in NMRiH; I don't believe the Maphack system would conflict in this case. Giving them a shout in the event that this case was overlooked.

I'll try to reproduce the crash with the information and example files you've provided.

There's another possibility, Use the hammer we can insert classname "npc_national_guard" into the map. but the nmrih engine does not have this entity.

dysphie commented 2 years ago

I am not experiencing crashes with my setup

"Maphack"
{
    "pre_entities"
    {
        $remove_all { "classname" "random_spawner" }
    }
}

My pre-merge tests didn't include maphacks, though (they require a .txt to exist), so it's possible there's some weird interaction between the two that slipped thru the cracks.

nosoop commented 2 years ago

I can't reproduce it with the official latest development build of SourceMod on Windows and the information provided (with dysphie's corrected file).

The crash mostten is getting happens within MapEntity_ParseAllEntities(), which calls IVEngineServer::GetMapEntitiesString(). Specifically, it crashes in MapEntity_PrecacheEntity(). So I suspect that the string it received from GetMapEntitiesString() (which was processed by the new entity lump code) wasn't correct in some way.

By any chance, you weren't testing Stripper:Source(pawn) or some other new plugin that uses the built-in entity lump stuff, were you? It looks like that plugin added safeguards to prevent additions of entities without classnames. The only safeguards in place in the lump serialization code in SourceMod is that empty entries are skipped; it's possible for a plugin to get the entity string into a state that doesn't parse correctly (removing required keys, inserting double quotes in the middle of keys / values), but that was also almost certainly possible with SDKHooks' OnLevelInit and Stripper:Source as well.

dysphie commented 1 year ago

I'm able to reproduce this in SM 1.12.6940 and this custom map

1) Place the following in maps/maphacks/nmo_tenkinoko_welkin_t1_3.txt

"Maphack"
{
    "pre_entities"
    {
        $remove { "targetname" "st4_fall_music2" } // ambient_generic
    }
}

2) map nmo_tenkinoko_welkin_t1_3

You should crash with "classname missing from entity!". Accelerator report: https://crash.limetech.org/t6mqyiyuhbfv

nosoop commented 1 year ago

Thanks for reproducing consistently; I've confirmed the issue and am looking into it.

At first glance it looks like the game doesn't like it when SourceMod passes control back with RETURN_META_VALUE_NEWPARAMS with the new string; not sure why that is at the moment.

I think in the worst case I'll have to put the writing capabilities behind a feature flag and disable it for games that ship with Maphack, but it should be fine for read-only purposes (aside from differences between what Entity Lump Manager receives and the post-processed string).

nosoop commented 1 year ago

On further testing, it looks like the bug is something really dumb.

… The game just doesn't like tabs as key / value delimiters. Replacing it with a space allows the map to load (on Windows at least; haven't spun up a Linux instance to confirm there). Maybe it's a specific quirk depending on if an entity string contains spaces — I haven't dug deep into the PrecachePointTemplates() function that it's actually failing on yet.

Can you test these builds to confirm if it resolves the issue without introducing any other regressions?

The build corresponds with https://github.com/nosoop/sourcemod/commits/entlump-fix, which are patched on top of 1.12.0.6923. They only contain binaries for SDK2013.

I'll still look into setting up some feature flagging via core.games as well if that fails, and I'll confirm that it doesn't now crash in TF2 at least.

felis-catus commented 1 year ago

Hey there, I'm the maintainer for NMRiH. Maphack rewrites the entire entdata buffer from scratch, it skips the \t characters to fix an issue with KeyValues::RecursiveSaveToFile() not respecting the ident level and including tabs anyway (IIRC this may have been the case, change was committed long time ago with vague description), it's a problem since entities are written one by one and then merged into final result. The system is jank and we didn't expect it would interfere with plugins this way - especially when it doesn't stomp the actual entity lump, my apologies for that!

Maphack is available on "No More Room in Hell" and "Pirates, Vikings, and Knights II", so these two games were the only ones affected.

nosoop commented 1 year ago

All good Felis; just found it amusing how some arbitrary decision I made on the separator had these sorts of effects.

Thanks for explaining the reasoning!

user4000 commented 1 year ago

Hello ! Can your plugin handle this

{ "origin" "-249 -455 35" "angles" "0 60 -90" "ammo" "90" "classname" "weapon_m4a1" } { "origin" "-311 -454 35" "angles" "0 120 -90" "ammo" "90" "classname" "weapon_aug" } { "origin" "-383 -441 33" "angles" "0 240 -90" "ammo" "90" "classname" "weapon_scout" }

as you can see ammo and classname are in the same line.