Skip to content

Commit

Permalink
Merge pull request #819 from openmultiplayer/amir/prevent-crash-amx_A…
Browse files Browse the repository at this point in the history 
…llot

Fix amx_Allot trying to access invalid memory when data size is higher than amx STK size
  • Loading branch information
@AmyrAhmady
AmyrAhmady authored Dec 29, 2023
2 parents 9483d4b + 34a714a commit ebb13f6
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 111 deletions.
287 changes: 178 additions & 109 deletions Server/Components/Pawn/Script/Script.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,114 +116,6 @@ PawnScript::~PawnScript()
tryLoad("");
}

int AMXAPI amx_NumPublics(AMX* amx, int* number)
{
AMX_HEADER* hdr = (AMX_HEADER*)amx->base;
assert(hdr != NULL);
assert(hdr->magic == AMX_MAGIC);
assert(hdr->publics <= hdr->natives);
*number = NUMENTRIES(hdr, publics, natives);
return AMX_ERR_NONE;
}

int AMXAPI amx_GetPublic(AMX* amx, int index, char* funcname)
{
AMX_HEADER* hdr;
AMX_FUNCPART* func;

hdr = (AMX_HEADER*)amx->base;
assert(hdr != NULL);
assert(hdr->magic == AMX_MAGIC);
assert(hdr->publics <= hdr->natives);
if (index >= (cell)NUMENTRIES(hdr, publics, natives))
return AMX_ERR_INDEX;

func = GETENTRY(hdr, publics, index);
strcpy(funcname, GETENTRYNAME(hdr, func));
return AMX_ERR_NONE;
}

__attribute__((noinline)) int amx_FindPublic_impl(AMX* amx, const char* name, int* index)
{
AMX_HEADER* hdr = (AMX_HEADER*)amx->base;
char* pname;
AMX_FUNCPART* func;

// Attempt to find index in publics cache
auto amxIter = cache.find(amx);
const bool cacheExists = amxIter != cache.end();
if (cacheExists)
{
const AMXCache& amxCache = *amxIter->second;
if (amxCache.inited)
{
auto lookupIter = amxCache.publics.find(name);
if (lookupIter != amxCache.publics.end())
{
// https://github.com/IllidanS4/pawn-conventions/blob/master/guidelines.md#do-not-rely-on-consistency
if (lookupIter->second < (cell)NUMENTRIES(hdr, publics, natives))
{
func = GETENTRY(hdr, publics, lookupIter->second);
pname = GETENTRYNAME(hdr, func);
if (!strcmp(name, pname))
{
*index = lookupIter->second;
return AMX_ERR_NONE;
}
}
}
}
}

// Cache miss; do the heavy search
int first, last, mid, result;

amx_NumPublics(amx, &last);
last--; /* last valid index is 1 less than the number of functions */
first = 0;
/* binary search */
while (first <= last)
{
mid = (first + last) / 2;
func = GETENTRY(hdr, publics, mid);
pname = GETENTRYNAME(hdr, func);
result = strcmp(pname, name);
if (result > 0)
{
last = mid - 1;
}
else if (result < 0)
{
first = mid + 1;
}
else
{
*index = mid;
// Cache public index
if (cacheExists)
{
AMXCache& amxCache = *amxIter->second;
if (amxCache.inited)
{
amxCache.publics[name] = mid;
}
}
return AMX_ERR_NONE;
} /* if */
} /* while */
/* not found, set to an invalid index, so amx_Exec() will fail */
*index = INT_MAX;
return AMX_ERR_NOTFOUND;
}

/// Pass-through to a noinline function to avoid adding complex instructions to the prologue that sampgdk can't handle
/// This should work in every case as both JMP and CALL are at least 5 bytes in size;
/// even in the minimal case it's guaranteed to contain a single JMP which is what sampgdk needs for a hook
__attribute__((optnone)) int AMXAPI amx_FindPublic(AMX* amx, const char* name, int* index)
{
return amx_FindPublic_impl(amx, name, index);
}

int AMXAPI amx_GetNativeByIndex(AMX const* amx, int index, AMX_NATIVE_INFO* ret)
{
AMX_HEADER*
Expand Down Expand Up @@ -387,7 +279,107 @@ static AMX_NATIVE findfunction(const char* name, const AMX_NATIVE_INFO* list, in
return NULL;
}

int AMXAPI amx_Register(AMX* amx, const AMX_NATIVE_INFO* list, int number)
__attribute__((noinline)) int AMXAPI amx_NumPublics_impl(AMX* amx, int* number)
{
AMX_HEADER* hdr = (AMX_HEADER*)amx->base;
assert(hdr != NULL);
assert(hdr->magic == AMX_MAGIC);
assert(hdr->publics <= hdr->natives);
*number = NUMENTRIES(hdr, publics, natives);
return AMX_ERR_NONE;
}

__attribute__((noinline)) int AMXAPI amx_GetPublic_impl(AMX* amx, int index, char* funcname)
{
AMX_HEADER* hdr;
AMX_FUNCPART* func;

hdr = (AMX_HEADER*)amx->base;
assert(hdr != NULL);
assert(hdr->magic == AMX_MAGIC);
assert(hdr->publics <= hdr->natives);
if (index >= (cell)NUMENTRIES(hdr, publics, natives))
return AMX_ERR_INDEX;

func = GETENTRY(hdr, publics, index);
strcpy(funcname, GETENTRYNAME(hdr, func));
return AMX_ERR_NONE;
}

__attribute__((noinline)) int amx_FindPublic_impl(AMX* amx, const char* name, int* index)
{
AMX_HEADER* hdr = (AMX_HEADER*)amx->base;
char* pname;
AMX_FUNCPART* func;

// Attempt to find index in publics cache
auto amxIter = cache.find(amx);
const bool cacheExists = amxIter != cache.end();
if (cacheExists)
{
const AMXCache& amxCache = *amxIter->second;
if (amxCache.inited)
{
auto lookupIter = amxCache.publics.find(name);
if (lookupIter != amxCache.publics.end())
{
// https://github.com/IllidanS4/pawn-conventions/blob/master/guidelines.md#do-not-rely-on-consistency
if (lookupIter->second < (cell)NUMENTRIES(hdr, publics, natives))
{
func = GETENTRY(hdr, publics, lookupIter->second);
pname = GETENTRYNAME(hdr, func);
if (!strcmp(name, pname))
{
*index = lookupIter->second;
return AMX_ERR_NONE;
}
}
}
}
}

// Cache miss; do the heavy search
int first, last, mid, result;

amx_NumPublics(amx, &last);
last--; /* last valid index is 1 less than the number of functions */
first = 0;
/* binary search */
while (first <= last)
{
mid = (first + last) / 2;
func = GETENTRY(hdr, publics, mid);
pname = GETENTRYNAME(hdr, func);
result = strcmp(pname, name);
if (result > 0)
{
last = mid - 1;
}
else if (result < 0)
{
first = mid + 1;
}
else
{
*index = mid;
// Cache public index
if (cacheExists)
{
AMXCache& amxCache = *amxIter->second;
if (amxCache.inited)
{
amxCache.publics[name] = mid;
}
}
return AMX_ERR_NONE;
} /* if */
} /* while */
/* not found, set to an invalid index, so amx_Exec() will fail */
*index = INT_MAX;
return AMX_ERR_NOTFOUND;
}

__attribute__((noinline)) int AMXAPI amx_Register_impl(AMX* amx, const AMX_NATIVE_INFO* list, int number)
{
AMX_FUNCPART* func;
AMX_HEADER* hdr;
Expand Down Expand Up @@ -453,3 +445,80 @@ int AMXAPI amx_Register(AMX* amx, const AMX_NATIVE_INFO* list, int number)
amx->flags |= AMX_FLAG_NTVREG;
return err;
}

__attribute__((noinline)) int AMXAPI amx_Allot_impl(AMX* amx, int cells, cell* amx_addr, cell** phys_addr)
{
AMX_HEADER* hdr;
unsigned char* data;

assert(amx != NULL);
hdr = (AMX_HEADER*)amx->base;
assert(hdr != NULL);
assert(hdr->magic == AMX_MAGIC);
data = (amx->data != NULL) ? amx->data : amx->base + (uintptr_t)hdr->dat;

if (amx->stk - amx->hea - cells * sizeof(cell) < STKMARGIN)
{
return AMX_ERR_MEMORY;
}

if (amx->stk < amx->hea + cells * sizeof(cell))
{
PawnManager::Get()->core->logLn(LogLevel::Error, "Unable to find enough memory for your data.");
PawnManager::Get()->core->logLn(LogLevel::Error, "Size: %i bytes, Available space: %i bytes, Need extra size: %i bytes",
int(amx->hea + cells * sizeof(cell)), amx->stk, int(amx->hea + cells * sizeof(cell) - amx->stk));
PawnManager::Get()->core->logLn(LogLevel::Error, "You can increase your available memory size by using `#pragma dynamic %i`.",
int(amx->hea / sizeof(cell) + cells));
return AMX_ERR_MEMORY;
}

assert(amx_addr != NULL);
assert(phys_addr != NULL);
*amx_addr = amx->hea;
*phys_addr = (cell*)(data + (uintptr_t)amx->hea);
amx->hea += cells * sizeof(cell);
return AMX_ERR_NONE;
}

__attribute__((noinline)) int AMXAPI amx_Release_impl(AMX* amx, cell amx_addr)
{
if (amx->hea > amx_addr)
{
amx->hea = amx_addr;
}
return AMX_ERR_NONE;
}

/// Pass-through to a noinline function to avoid adding complex instructions to the prologue that sampgdk can't handle
/// This should work in every case as both JMP and CALL are at least 5 bytes in size;
/// even in the minimal case it's guaranteed to contain a single JMP which is what sampgdk needs for a hook
/// This applies to all functions below.
__attribute__((optnone)) int AMXAPI amx_NumPublics(AMX* amx, int* number)
{
return amx_NumPublics_impl(amx, number);
}

__attribute__((optnone)) int AMXAPI amx_GetPublic(AMX* amx, int index, char* funcname)
{
return amx_GetPublic_impl(amx, index, funcname);
}

__attribute__((optnone)) int AMXAPI amx_FindPublic(AMX* amx, const char* name, int* index)
{
return amx_FindPublic_impl(amx, name, index);
}

__attribute__((optnone)) int AMXAPI amx_Release(AMX* amx, cell amx_addr)
{
return amx_Release_impl(amx, amx_addr);
}

__attribute__((optnone)) int AMXAPI amx_Allot(AMX* amx, int cells, cell* amx_addr, cell** phys_addr)
{
return amx_Allot_impl(amx, cells, amx_addr, phys_addr);
}

__attribute__((optnone)) int AMXAPI amx_Register(AMX* amx, const AMX_NATIVE_INFO* list, int number)
{
return amx_Register_impl(amx, list, number);
}
4 changes: 3 additions & 1 deletion lib/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ if(BUILD_SERVER)
-DAMX_FILENO_CHECKS

# Disable default amx_FindPublic implementation because we want our own more performant one
AMX_ALIGN AMX_ALLOT AMX_CLEANUP AMX_CLONE AMX_DEFCALLBACK AMX_EXEC AMX_FLAGS AMX_GETADDR
# Also disable amx_Allot because we want to check if we have enough memory when we are passing data
# Which is more than available memory, so instead of a crash, we let user know about it.
AMX_ALIGN AMX_CLEANUP AMX_CLONE AMX_DEFCALLBACK AMX_FLAGS AMX_GETADDR
AMX_INIT AMX_MEMINFO AMX_NAMELENGTH AMX_NATIVEINFO AMX_PUSHXXX
AMX_RAISEERROR AMX_REGISTER AMX_SETCALLBACK AMX_SETDEBUGHOOK
AMX_XXXNATIVES AMX_XXXPUBVARS AMX_XXXSTRING AMX_XXXTAGS AMX_XXXUSERDATA AMX_UTF8XXX
Expand Down
2 changes: 1 addition & 1 deletion lib/pawn
Submodule pawn updated 3 files
+0 −3 source/amx/amx.c
+1 −0 source/amx/amx.h
+1 −1 source/amx/amxfile.c

0 comments on commit ebb13f6

Please sign in to comment.