Skip to content

Commit

Permalink
Fix buffer overflow issues in GetNameAndDamage (#2459)
Browse files Browse the repository at this point in the history
This also makes the crashes from issue #2262 stop, but as the recent comment in that issue says, there will soon be another tweak to increase the buffer size as well
  • Loading branch information
Pirulax committed Dec 23, 2021
1 parent 7b15218 commit 1129399
Showing 1 changed file with 41 additions and 6 deletions.
47 changes: 41 additions & 6 deletions Client/game_sa/CFileLoaderSA.cpp
Expand Up @@ -36,9 +36,44 @@ static char* GetFrameNodeName(RwFrame* frame)
return ((char*(__cdecl*)(RwFrame*))0x72FB30)(frame);
}

static void GetNameAndDamage(char const* nodeName, char* outName, bool& outDamage)
{
return ((void(__cdecl*)(char const*, char*, bool&))0x5370A0)(nodeName, outName, outDamage);
// Originally there was a possibility for this function to cause buffer overflow
// It should be fixed here.
template<size_t OutBuffSize>
void GetNameAndDamage(const char* nodeName, char(&outName)[OutBuffSize], bool& outDamage) {
const auto nodeNameLen = strlen(nodeName);

const auto NodeNameEndsWith = [=](const char* with) {
const auto withLen = strlen(with);
dassert(withLen <= nodeNameLen);
return withLen >= nodeNameLen /*dont bother checking otherwise, because it might cause a crash*/
&& strncmp(nodeName + nodeNameLen - withLen, with, withLen) == 0;
};

// Copy `nodeName` into `outName` with `off` trimmed from the end
// Eg.: `dmg_dam` with `off = 4` becomes `dmg`
const auto TerminatedCopy = [&](size_t off) {
dassert(nodeNameLen - off < OutBuffSize);

strncpy_s(outName, OutBuffSize, nodeName, nodeNameLen - off);
outName[nodeNameLen - off] = 0;
};

if (NodeNameEndsWith("_dam"))
{
outDamage = true;
TerminatedCopy(sizeof("_dam") - 1);
}
else
{
outDamage = false;
if (NodeNameEndsWith("_l0") || NodeNameEndsWith("_L0")) {
TerminatedCopy(sizeof("_l0") - 1);
} else {
dassert(nodeNameLen < OutBuffSize);
//strcpy_s(outName, nodeName);
strncpy_s(outName, OutBuffSize, nodeName, strlen(nodeName));
}
}
}

static void CVisibilityPlugins_SetAtomicRenderCallback(RpAtomic* pRpAtomic, RpAtomic* (*renderCB)(RpAtomic*))
Expand Down Expand Up @@ -94,7 +129,7 @@ bool CFileLoader_LoadAtomicFile(RwStream* stream, unsigned int modelId)
relatedModelInfo.pClump = pReadClump;
relatedModelInfo.bDeleteOldRwObject = false;

RpClumpForAllAtomics(pReadClump, (RpClumpForAllAtomicsCB_t)CFileLoader_SetRelatedModelInfoCB, &relatedModelInfo);
RpClumpForAllAtomics(pReadClump, reinterpret_cast<RpClumpForAllAtomicsCB_t>(CFileLoader_SetRelatedModelInfoCB), &relatedModelInfo);
RpClumpDestroy(pReadClump);
}

Expand All @@ -118,7 +153,7 @@ RpAtomic* CFileLoader_SetRelatedModelInfoCB(RpAtomic* atomic, SRelatedModelInfo*
RwFrame* pOldFrame = reinterpret_cast<RwFrame*>(atomic->object.object.parent);
char* frameNodeName = GetFrameNodeName(pOldFrame);
bool bDamage = false;
GetNameAndDamage(frameNodeName, (char*)&name, bDamage);
GetNameAndDamage(frameNodeName, name, bDamage);
CVisibilityPlugins_SetAtomicRenderCallback(atomic, 0);

RpAtomic* pOldAtomic = reinterpret_cast<RpAtomic*>(pBaseModelInfo->pRwObject);
Expand Down Expand Up @@ -163,7 +198,7 @@ CEntitySAInterface* CFileLoader_LoadObjectInstance(const char* szLine)

/*
A quaternion is must be normalized. GTA is relying on an internal R* exporter and everything is OK,
but custom exporters might not contain the normalization. And we must do it yourself.
but custom exporters might not contain the normalization. And we must do it instead.
*/
const float fLenSq = inst.rotation.LengthSquared();
if (fLenSq > 0.0f && std::fabs(fLenSq - 1.0f) > std::numeric_limits<float>::epsilon())
Expand Down

0 comments on commit 1129399

Please sign in to comment.