denuvo-ref/HyperDbg
1
0
Fork 0
mirror of https://github.com/HyperDbg/HyperDbg synced 2026-08-15 06:29:09 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

Fix memory-safety and robustness issues in script engine and PCI ID parser

Browse source 
Code audit of the script engine's scanner/token handling and of the PCI ID
database parser. Each of the issues below was reproduced against the current
code before the fix and re-checked afterwards.

script-engine/scanner.c

  * An unterminated string literal ("abc or L"abc) hung the scanner in an
    endless loop: sgetc() returns EOF without consuming input, and neither
    string loop tested for it, so the token grew until allocation failed.
    Both loops now stop at EOF and report the token as UNKNOWN.

script-engine/common.c

  * AppendByte()/AppendWchar() doubled Token->MaxLen before checking whether
    the larger buffer was actually allocated. After a failed allocation MaxLen
    described memory that did not exist and the next append wrote past the end
    of the old buffer. MaxLen is now committed only on success.

  * CopyToken() allocated strlen(Value) + 1 bytes but carried over the source
    token's Len and MaxLen, so the copy's advertised capacity did not match its
    allocation, and WSTRING payloads were truncated at their first embedded
    null byte. The copy is now sized from Len/MaxLen and copied by length, with
    a fallback to the string length for the grammar tokens in parse-table.c,
    which only initialize Type and Value.

  * NewToken() set MaxLen to the value length, which is zero for an empty
    value. The 'Len >= MaxLen - 1' test in the append routines is unsigned, so
    a zero MaxLen wrapped and disabled buffer growth entirely.

  * IsUnderscore() tested 'c >= '_'', which also accepted the backtick, the
    lowercase letters, '{', '|', '}', '~' and DEL. Register scanning uses it,
    so '@rax|1' was lexed as one malformed register name instead of a register,
    an operator and a number. The pseudo-register path already compared against
    '_' directly.

  * NewTokenList() did not check the allocation of its Head buffer.

  * NewTemp() kept the last handed-out id in a static, so an exhausted temp
    list produced a token aliasing a temporary still in use, and it derived
    MaxTempNumber from an out-of-range index. It also dereferenced the new
    token without a null check.

  * FreeTemp() indexed the MAX_TEMP_COUNT-entry map with an unchecked value
    parsed out of the token text.

  * RotateLeftStringOnce() wrote to str[-1] when handed an empty string.

libhyperdbg/debugger/misc/pci-id.cpp

  * The database file was read into a malloc(Length) buffer that was never
    null-terminated, while ReadLine() walks it with strchr(). Looking up an
    absent vendor scans to the end and reads past the allocation.

  * The matched Vendor was allocated with malloc() and its Devices list head
    was only assigned once a device line was parsed, so a vendor with no
    device entries left it uninitialized and FreeVendor() walked a garbage
    pointer.

  * FreeVendor() released the device and subdevice lists but never the Vendor
    itself, leaking one per lookup for every enumerated PCI device.

  * The file handle leaked when the buffer allocation failed, ftell() and
    fread() results were unused, and several error paths leaked the Vendor or
    the not-yet-linked Device/SubDevice.

  * strncmp() compared sizeof(VendorId) bytes, which is the size of the
    pointer rather than the length of a vendor id.

  * ReadLine() passed an unclamped count to strncpy_s(), which triggers the
    invalid parameter handler for a line longer than the destination.

  * GetVendorById() ignored the GetModuleFileName() result and overwrote the
    tail of the path buffer without checking the room left in it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W3C1DuhHtqK64eEkHjKCHM
This commit is contained in:
Claude 2026-08-01 14:22:52 +00:00
parent 056793954e
commit 1abdde7702
No known key found for this signature in database
3 changed files with 221 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

95
hyperdbg/libhyperdbg/code/debugger/misc/pci-id.cpp
View file

@ -73,7 +73,18 @@ ReadLine(CHAR * DestBuffer, UINT64 CharLimit, CHAR ** SrcBuffer)
}
else
{
strncpy_s(DestBuffer, CharLimit, *SrcBuffer, (Line - *SrcBuffer));
//
// The copy length is clamped to the destination, otherwise a line longer than
// CharLimit makes strncpy_s() invoke the invalid parameter handler
//
SIZE_T LineLength = (SIZE_T)(Line - *SrcBuffer);
if (LineLength > CharLimit - 1)
{
LineLength = (SIZE_T)(CharLimit - 1);
}
strncpy_s(DestBuffer, (rsize_t)CharLimit, *SrcBuffer, LineLength);
*SrcBuffer += (Line - *SrcBuffer + 1);
return *SrcBuffer;
}
@ -108,17 +119,35 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
}
fseek(f, 0, SEEK_END);
Length = ftell(f);
PciIdDatabaseBuffer = (CHAR *)malloc(Length);
LONG FileSize = ftell(f);
if (FileSize < 0)
{
ShowMessages("Error: Cannot determine the size of file '%s': error %d\n", Filename, errno);
fclose(f);
return NULL;
}
Length = (SIZE_T)FileSize;
//
// One extra byte is allocated for the null terminator, as the buffer is later
// walked with strchr() by ReadLine() and would otherwise be read past its end
//
PciIdDatabaseBuffer = (CHAR *)malloc(Length + 1);
if (!PciIdDatabaseBuffer)
{
fclose(f);
return NULL;
}
fseek(f, 0, SEEK_SET);
fread(PciIdDatabaseBuffer, 1, Length, f);
SIZE_T BytesRead = fread(PciIdDatabaseBuffer, 1, Length, f);
fclose(f);
PciIdDatabaseBuffer[BytesRead] = '\0';
}
PciIdDbBufPtr = PciIdDatabaseBuffer;
@ -142,9 +171,19 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
snprintf(FormatStr, sizeof(FormatStr), "%%4s %%%d[^\n]", PCI_NAME_STR_LENGTH); // FormatStr = "%4s %PCI_NAME_STR_LENGTH[^\n]"
if (sscanf(Line, FormatStr, VendorBuf, VendorNameBuf) == 2)
{
if (strncmp(VendorBuf, VendorId, sizeof(VendorId)) == 0)
//
// VendorId is a pointer, so sizeof() on it yielded the pointer size
// rather than the length of a PCI vendor id
//
if (strncmp(VendorBuf, VendorId, PCI_ID_AS_STR_LENGTH) == 0)
{
MatchedVendor = (Vendor *)malloc(sizeof(Vendor));
//
// calloc() so that the Devices list head starts out empty: it is
// only assigned once a device line is parsed, and FreeVendor()
// would otherwise walk an uninitialized pointer for a vendor that
// has no devices listed
//
MatchedVendor = (Vendor *)calloc(1, sizeof(Vendor));
if (!MatchedVendor)
{
return NULL;
@ -153,6 +192,7 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
INT Result = sscanf(VendorBuf, "%hx", &(MatchedVendor->VendorId));
if (Result != 1)
{
FreeVendor(MatchedVendor);
return NULL;
}
strncpy_s(MatchedVendor->VendorName, sizeof(MatchedVendor->VendorName), TrimWhitespace(VendorNameBuf, PCI_NAME_STR_LENGTH), _TRUNCATE);
@ -178,6 +218,11 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
int Result = sscanf(DeviceBuf, "%hx", &(NewDevice->DeviceId));
if (Result != 1)
{
//
// NewDevice is not linked into the vendor's list yet, so it has to
// be released separately from FreeVendor()
//
free(NewDevice);
FreeVendor(MatchedVendor);
return NULL;
}
@ -216,6 +261,11 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
int Result = sscanf(SubVendorBuf, "%hx", &NewSubDevice->SubVendorId);
if (Result != 1)
{
//
// NewSubDevice is not linked into the device's list yet, so it has
// to be released separately from FreeVendor()
//
free(NewSubDevice);
FreeVendor(MatchedVendor);
return NULL;
}
@ -223,6 +273,7 @@ GetVendorByIdStr(const CHAR * Filename, const CHAR * VendorId)
Result = sscanf(SubDeviceBuf, "%hx", &NewSubDevice->SubDeviceId);
if (Result != 1)
{
free(NewSubDevice);
FreeVendor(MatchedVendor);
return NULL;
}
@ -278,6 +329,13 @@ FreeVendor(Vendor * VendorToFree)
free(CurrentDevice);
CurrentDevice = NextDevice;
}
//
// The Vendor itself is allocated by GetVendorByIdStr() and was previously never
// released, leaking one Vendor per call for every PCI device that got enumerated
//
VendorToFree->Devices = NULL;
free(VendorToFree);
}
/**
@ -309,7 +367,17 @@ GetVendorById(UINT16 VendorId)
HMODULE hModule = GetModuleHandle(NULL);
snprintf(VendorIdAsStr, sizeof(VendorIdAsStr), "%04X", VendorId);
GetModuleFileName(hModule, ExecutablePath, sizeof(ExecutablePath));
DWORD PathLength = GetModuleFileName(hModule, ExecutablePath, sizeof(ExecutablePath));
//
// A zero length means the call failed; a length equal to the buffer size means the
// path was truncated and, on older Windows versions, left without a null terminator
//
if (PathLength == 0 || PathLength >= sizeof(ExecutablePath))
{
return NULL;
}
// Extract executable name
CHAR * ExecutableName = strrchr(ExecutablePath, '\\');
@ -323,7 +391,18 @@ GetVendorById(UINT16 VendorId)
}
// Swap executable name for PCI_ID_DATABASE_PATH
strncpy(ExecutableName, PCI_ID_DATABASE_PATH, sizeof(PCI_ID_DATABASE_PATH));
//
// The database path can be longer than the executable name it replaces, so the
// room left in ExecutablePath is checked before overwriting the tail
//
SIZE_T RemainingSpace = sizeof(ExecutablePath) - (SIZE_T)(ExecutableName - ExecutablePath);
if (RemainingSpace < sizeof(PCI_ID_DATABASE_PATH))
{
return NULL;
}
memcpy(ExecutableName, PCI_ID_DATABASE_PATH, sizeof(PCI_ID_DATABASE_PATH));
return GetVendorByIdStr(ExecutablePath, ToLower(VendorIdAsStr));
}

133
hyperdbg/script-engine/code/common.c
View file

@ -86,10 +86,14 @@ NewToken(SCRIPT_ENGINE_TOKEN_TYPE Type, char * Value)
//
// Init fields
//
// Note that MaxLen is never allowed to be zero here. AppendByte()/AppendWchar()
// test for a full buffer with 'Len >= MaxLen - 1' on an unsigned type, so a zero
// MaxLen would wrap around and let those routines write past the allocation
//
unsigned int Len = (unsigned int)strlen(Value);
Token->Type = Type;
Token->Len = Len;
Token->MaxLen = Len;
Token->MaxLen = Len > TOKEN_VALUE_MAX_LEN ? Len : TOKEN_VALUE_MAX_LEN;
Token->Value = (char *)calloc(Token->MaxLen + 1, sizeof(char));
Token->VariableType = (VARIABLE_TYPE *)VARIABLE_TYPE_LONG;
Token->VariableMemoryIdx = 0;
@ -254,11 +258,16 @@ AppendByte(PSCRIPT_ENGINE_TOKEN Token, char c)
//
// Double the length of the allocated space for the string
//
Token->MaxLen *= 2;
char * NewValue = (char *)calloc(Token->MaxLen + 1, sizeof(char));
unsigned int NewMaxLen = Token->MaxLen * 2;
char * NewValue = (char *)calloc(NewMaxLen + 1, sizeof(char));
if (NewValue == NULL)
{
//
// MaxLen is only committed once the bigger buffer is in hand, otherwise
// it would describe a buffer that was never allocated and the next call
// would consider the (still small) buffer to have free space in it
//
printf("err, could not allocate buffer");
return;
}
@ -268,7 +277,8 @@ AppendByte(PSCRIPT_ENGINE_TOKEN Token, char c)
//
memcpy(NewValue, Token->Value, Token->Len);
free(Token->Value);
Token->Value = NewValue;
Token->Value = NewValue;
Token->MaxLen = NewMaxLen;
}
//
@ -296,11 +306,15 @@ AppendWchar(PSCRIPT_ENGINE_TOKEN Token, wchar_t c)
//
// Double the length of the allocated space for the wstring
//
Token->MaxLen *= 2;
char * NewValue = (char *)calloc(Token->MaxLen + 2, sizeof(char));
unsigned int NewMaxLen = Token->MaxLen * 2;
char * NewValue = (char *)calloc(NewMaxLen + 2, sizeof(char));
if (NewValue == NULL)
{
//
// Keep MaxLen describing the buffer that is actually allocated, see the
// matching comment in AppendByte()
//
printf("err, could not allocate buffer");
return;
}
@ -310,7 +324,8 @@ AppendWchar(PSCRIPT_ENGINE_TOKEN Token, wchar_t c)
//
memcpy(NewValue, Token->Value, Token->Len);
free(Token->Value);
Token->Value = NewValue;
Token->Value = NewValue;
Token->MaxLen = NewMaxLen;
}
//
@ -339,11 +354,35 @@ CopyToken(PSCRIPT_ENGINE_TOKEN Token)
return NULL;
}
TokenCopy->Type = Token->Type;
TokenCopy->MaxLen = Token->MaxLen;
TokenCopy->Len = Token->Len;
TokenCopy->Value = (char *)calloc(strlen(Token->Value) + 1, sizeof(char));
TokenCopy->VariableType = Token->VariableType;
//
// The number of bytes to copy is the larger of Len and the string length:
//
// - WSTRING tokens hold UTF-16 data whose embedded null bytes make strlen()
// (and strcpy()) stop early, so Len is the meaningful size for them
// - tokens taken from the static grammar tables in parse-table.c only have
// their Type and Value initialized, leaving Len at zero, so the string
// length is the meaningful size for those
//
unsigned int ValueLen = (unsigned int)strlen(Token->Value);
unsigned int CopyLen = Token->Len > ValueLen ? Token->Len : ValueLen;
unsigned int MaxLen = Token->MaxLen > CopyLen ? Token->MaxLen : CopyLen;
if (MaxLen < TOKEN_VALUE_MAX_LEN)
{
MaxLen = TOKEN_VALUE_MAX_LEN;
}
TokenCopy->Type = Token->Type;
//
// MaxLen describes the buffer that is actually allocated below. It used to be
// copied verbatim from the source token while the allocation was sized from the
// string length, so the two could disagree and let AppendByte()/AppendWchar()
// write past the end of the copy
//
TokenCopy->MaxLen = MaxLen;
TokenCopy->Len = Token->Len;
TokenCopy->Value = (char *)calloc(MaxLen + 2, sizeof(char));
TokenCopy->VariableType = Token->VariableType;
TokenCopy->VariableMemoryIdx = Token->VariableMemoryIdx;
TokenCopy->AddressSpace = Token->AddressSpace;
TokenCopy->IsAddress = Token->IsAddress;
@ -357,7 +396,7 @@ CopyToken(PSCRIPT_ENGINE_TOKEN Token)
return NULL;
}
strcpy(TokenCopy->Value, Token->Value);
memcpy(TokenCopy->Value, Token->Value, CopyLen);
return TokenCopy;
}
@ -396,6 +435,15 @@ NewTokenList(void)
//
TokenList->Head = (PSCRIPT_ENGINE_TOKEN *)malloc(TokenList->Size * sizeof(PSCRIPT_ENGINE_TOKEN));
if (TokenList->Head == NULL)
{
//
// There was an error allocating buffer
//
free(TokenList);
return NULL;
}
return TokenList;
}
@ -612,7 +660,7 @@ IsLetter(char c)
char
IsUnderscore(char c)
{
if (c >= '_')
if (c == '_')
return 1;
else
{
@ -661,8 +709,8 @@ IsOctal(char c)
PSCRIPT_ENGINE_TOKEN
NewTemp(PSCRIPT_ENGINE_ERROR_TYPE Error)
{
static unsigned int TempID = 0;
int i;
unsigned int TempID = 0;
int i;
for (i = 0; i < MAX_TEMP_COUNT; i++)
{
if (CurrentUserDefinedFunction->TempMap[i] == 0)
@ -674,15 +722,42 @@ NewTemp(PSCRIPT_ENGINE_ERROR_TYPE Error)
}
if (i == MAX_TEMP_COUNT)
{
//
// No slot is free. The error is reported to the caller, which aborts the
// code generation. A token is still returned so that the (many) call sites
// that dereference the result before testing *Error keep working
//
// TempID is deliberately a plain local rather than a static: when it was
// static it kept the id handed out by the previous call, so an exhausted
// temp list produced a token aliasing a temporary that was still in use
//
*Error = SCRIPT_ENGINE_ERROR_TEMP_LIST_FULL;
}
PSCRIPT_ENGINE_TOKEN Temp = NewUnknownToken();
char TempValue[8];
if (Temp == NULL)
{
//
// There was an error allocating the token, so release the reserved slot
//
if (i != MAX_TEMP_COUNT)
{
CurrentUserDefinedFunction->TempMap[i] = 0;
}
return NULL;
}
char TempValue[8];
sprintf(TempValue, "%d", TempID);
strcpy(Temp->Value, TempValue);
Temp->Type = TEMP;
if (CurrentUserDefinedFunction->MaxTempNumber < (i + 1))
//
// 'i' is only a valid temporary index when a free slot was actually found,
// otherwise this would size the frame for MAX_TEMP_COUNT + 1 temporaries
//
if (i != MAX_TEMP_COUNT && CurrentUserDefinedFunction->MaxTempNumber < (unsigned long long)(i + 1))
{
CurrentUserDefinedFunction->MaxTempNumber = i + 1;
}
@ -699,8 +774,18 @@ NewTemp(PSCRIPT_ENGINE_ERROR_TYPE Error)
VOID
FreeTemp(PSCRIPT_ENGINE_TOKEN Temp)
{
if (Temp->Type != TEMP && Temp->Type != DEFERENCE_TEMP)
{
return;
}
//
// The index is derived from the token's textual value, so it is range-checked
// before indexing the MAX_TEMP_COUNT-entry map
//
INT Id = (INT)DecimalToInt(Temp->Value);
if (Temp->Type == TEMP || Temp->Type == DEFERENCE_TEMP)
if (Id >= 0 && Id < MAX_TEMP_COUNT)
{
CurrentUserDefinedFunction->TempMap[Id] = 0;
}
@ -1544,6 +1629,16 @@ RotateLeftStringOnce(char * str)
{
INT Length = (INT)strlen(str);
CHAR Temp = str[0];
//
// An empty string has nothing to rotate, and writing the saved character back
// would land on str[-1]
//
if (Length == 0)
{
return;
}
for (int i = 0; i < (Length - 1); i++)
{
str[i] = str[i + 1];

20
hyperdbg/script-engine/code/scanner.c
View file

@ -32,6 +32,16 @@ GetToken(char * c, char * str)
{
*c = sgetc(str);
//
// An unterminated string literal would otherwise spin here forever,
// since sgetc() keeps returning EOF without consuming any input
//
if ((int)*c == EOF)
{
Token->Type = UNKNOWN;
return Token;
}
if (*c == '\\')
{
*c = sgetc(str);
@ -650,6 +660,16 @@ GetToken(char * c, char * str)
{
*c = sgetc(str);
//
// An unterminated wide string literal would otherwise spin here
// forever, since sgetc() keeps returning EOF without consuming input
//
if ((int)*c == EOF)
{
Token->Type = UNKNOWN;
return Token;
}
if (*c == '\\')
{
*c = sgetc(str);