Fix FormatIface out-of-bounds writes and the InterfaceSearch hang - #284
Open
makadore wants to merge 3 commits into
Open
Fix FormatIface out-of-bounds writes and the InterfaceSearch hang#284makadore wants to merge 3 commits into
makadore wants to merge 3 commits into
Conversation
Two problems, both reachable through ISmmAPI::FormatIface, which documents that the caller supplies strlen(iface)+4. With no trailing digits the scan stopped on the last character rather than past it, so the version field was written one byte early and overwrote it: "ServerGameDLL" became "ServerGameDL001", and a name shorter than the field disappeared entirely, "X" becoming "001". With a trailing run shorter than three digits the size check was too weak. It only demanded maxlength > strlen, while the write needs three digits and a terminator from where the digits start, so "ServerGameDLL5" with a 15-byte buffer wrote two bytes past the end. Checked against a guard byte past the buffer, both cases are gone; the check is now the one the write actually needs. Names ending in the usual three digits are byte-for-byte unchanged, which is every interface metamod looks up itself, including through InterfaceSearch, which passes strlen+1. The version field is also formatted through a scratch buffer now. Anything wider than three digits is still truncated exactly as before, but the compiler can see the bound, so this no longer fails the build under -Werror=format-truncation.
std::unique_ptr<FILE, decltype(&::fclose)> does not compile under GCC: glibc attributes fclose with __nonnull__ and __wur, and an attributed type as a template argument is -Werror=ignored-attributes. Spelling the deleter out avoids it. CI builds with clang only, so this went unnoticed. Separately, every failure path in _Load leaves a reason in the error buffer except the one where the plugin's own Load() returns false, which is left as the empty string _Load starts it as. A plugin that refuses without setting a message is then reported with no reason at all, as in alliedmodders#231.
FormatIface returns -1 when the version field will not fit the buffer it is given, and the search loop used its return directly as the condition. -1 is true, so the loop calls the factory with an unchanged name and asks FormatIface again, forever. InterfaceSearch passes strlen+1, which is enough only for a name already ending in three digits, so any other name hangs. "ServerGameDLL" and "ServerGameDLL5" both spin today; with a guard byte and a call cap they never come back. Taking only a positive result as "keep going" ends the loop instead, and names ending in three digits, which is everything metamod looks up itself, behave exactly as before. Worth noting separately: strlen+1 does not meet the strlen+4 that ISmmAPI::FormatIface documents, which is why the search gives up on names without a three digit suffix rather than counting up from 001 as the description suggests. Left alone here, since changing it changes which interfaces get probed.
makadore
marked this pull request as draft
August 24, 2026 22:36
makadore
marked this pull request as ready for review
August 24, 2026 22:58
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FormatIfacewrites out of bounds and mangles names, andInterfaceSearchon top of it can spin forever. Neither depends on the compiler; both reproduce under clang and GCC.Names without a three digit suffix lose a character. The backwards scan stops on the last character instead of past it, so the version field is written one byte early and overwrites it. With the
strlen(iface)+4bufferISmmAPI::FormatIfacedocuments,ServerGameDLLcomes back asServerGameDL001, and a name shorter than the field is replaced outright —Xbecomes001.A trailing run shorter than three digits writes past the end. The size check asks for
maxlength > strlen, while the write needs three digits and a terminator from where the digits start.ServerGameDLL5into a 15-byte buffer puts two bytes past it; checked against a guard byte,ServerGameDLL55andA1do the same.InterfaceSearchhangs on anything but a three digit suffix. It uses the return ofFormatIfacedirectly as its loop condition.FormatIfacereturns-1when the version will not fit,-1is true, so the loop calls the factory with an unchanged name and asks again, forever. It passesstrlen+1, which is only ever enough for a name that already ends in three digits, soInterfaceSearch("ServerGameDLL", ...)andInterfaceSearch("ServerGameDLL5", ...)never return. Against a call cap:ServerGameDLL005->...008VEngineServer021->...023ServerGameDLLServerGameDLL5ServerGameDLL55Tightening the size check makes
FormatIfacereturn-1in one more case, which without the loop fix turns that last row into a hang too, so the two belong together.Names ending in the usual three digits come out byte-for-byte identical, which covers every interface Metamod looks up itself. Verified on a HL2DM dedicated server: Metamod loads and SourceMod loads under it, identically before and after.
The third commit is separate: every failure path in
_Loadleaves a reason in the error buffer except the one where the plugin's ownLoad()returns false, so a plugin that refuses without setting a message is reported with no reason at all. With a plugin that does exactly that, on the same server:Fixes #231.
One of the changes is only about GCC and you may not want it:
std::unique_ptr<FILE, decltype(&::fclose)>trips-Werror=ignored-attributes, since glibc attributesfcloseand an attributed type cannot be a template argument. CI has only ever run clang, so GCC is a configurationAMBuildScripthandles but nothing tests, andmasterdoes not build with it —FormatIface'ssnprintfinto a four-byte field was the other failure, which the fix above resolves anyway by formatting through a scratch buffer, truncating exactly as before. Happy to drop that commit if GCC is not something you want to carry.Built and tested both ways: clang 18.1 and GCC 13.3, x86, against hl2sdk-hl2dm.