Skip to content

Fix bugs and add code review documentation #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
brofield merged 3 commits into from
Nov 13, 2025
Merged

Fix bugs and add code review documentation #81

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
229a429
Fix critical bugs and add comprehensive code review documentation
claude Nov 13, 2025
6e17e1a
Remove findings document
brofield Nov 13, 2025
69f3734
Remove usage of SIZE_MAX
brofield Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 39 additions & 18 deletions SimpleIni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,9 @@

@section notes NOTES

- The maximum supported file size is 1 GiB (SI_MAX_FILE_SIZE). Files larger
than this will be rejected with SI_FILE error to prevent excessive memory
allocation and potential denial of service attacks.
- To load UTF-8 data on Windows 95, you need to use Microsoft Layer for
Unicode, or SI_CONVERT_GENERIC, or SI_CONVERT_ICU.
- When using SI_CONVERT_GENERIC, ConvertUTF.c must be compiled and linked.
Expand Down Expand Up @@ -261,6 +264,10 @@ constexpr int SI_FAIL = -1; //!< Generic failure
constexpr int SI_NOMEM = -2; //!< Out of memory error
constexpr int SI_FILE = -3; //!< File error (see errno for detail error)

//! Maximum supported file size (1 GiB). Files larger than this will be rejected
//! to prevent excessive memory allocation and potential denial of service.
constexpr size_t SI_MAX_FILE_SIZE = 1024ULL * 1024ULL * 1024ULL;

#define SI_UTF8_SIGNATURE "\xEF\xBB\xBF"

#ifdef _WIN32
Expand Down Expand Up @@ -357,7 +364,7 @@ class CSimpleIniTempl
if (lhs.nOrder != rhs.nOrder) {
return lhs.nOrder < rhs.nOrder;
}
return KeyOrder()(lhs.pItem, rhs.pItem);
return KeyOrder()(lhs, rhs);
}
};
};
Expand Down Expand Up @@ -1457,9 +1464,14 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::LoadFile(
if (lSize == 0) {
return SI_OK;
}


// check file size is within supported limits (SI_MAX_FILE_SIZE)
if (static_cast<size_t>(lSize) > SI_MAX_FILE_SIZE) {
return SI_FILE;
}

// allocate and ensure NULL terminated
char * pData = new(std::nothrow) char[lSize+static_cast<size_t>(1)];
char * pData = new(std::nothrow) char[static_cast<size_t>(lSize) + 1];
if (!pData) {
return SI_NOMEM;
}
Expand Down Expand Up @@ -1511,13 +1523,18 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::LoadData(
return SI_FAIL;
}

// check converted data size is within supported limits (SI_MAX_FILE_SIZE)
if (uLen >= (SI_MAX_FILE_SIZE / sizeof(SI_CHAR))) {
return SI_FILE;
}

// allocate memory for the data, ensure that there is a NULL
// terminator wherever the converted data ends
SI_CHAR * pData = new(std::nothrow) SI_CHAR[uLen+1];
SI_CHAR * pData = new(std::nothrow) SI_CHAR[uLen + 1];
if (!pData) {
return SI_NOMEM;
}
memset(pData, 0, sizeof(SI_CHAR)*(uLen+1));
memset(pData, 0, sizeof(SI_CHAR) * (uLen + 1));

// convert the data
if (!converter.ConvertFromStore(a_pData, a_uDataLen, pData, uLen)) {
Expand Down Expand Up @@ -1795,15 +1812,16 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::IsMultiLineData(
}

// embedded newlines
const SI_CHAR * pStart = a_pData;
while (*a_pData) {
if (IsNewLineChar(*a_pData)) {
return true;
}
++a_pData;
}

// check for suffix
if (IsSpace(*--a_pData)) {
// check for suffix (ensure we don't go before start of string)
if (a_pData > pStart && IsSpace(*(a_pData - 1))) {
return true;
}

Expand All @@ -1816,7 +1834,7 @@ CSimpleIniTempl<SI_CHAR, SI_STRLESS, SI_CONVERTER>::IsSingleLineQuotedValue(
const SI_CHAR* a_pData
) const
{
// data needs quoting if it starts or ends with whitespace
// data needs quoting if it starts or ends with whitespace
// and doesn't have embedded newlines

// empty string
Expand All @@ -1830,15 +1848,16 @@ CSimpleIniTempl<SI_CHAR, SI_STRLESS, SI_CONVERTER>::IsSingleLineQuotedValue(
}

// embedded newlines
const SI_CHAR * pStart = a_pData;
while (*a_pData) {
if (IsNewLineChar(*a_pData)) {
return false;
}
++a_pData;
}

// check for suffix
if (IsSpace(*--a_pData)) {
// check for suffix (ensure we don't go before start of string)
if (a_pData > pStart && IsSpace(*(a_pData - 1))) {
return true;
}

Expand Down Expand Up @@ -2083,7 +2102,8 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::AddEntry(
if (pComment) {
DeleteString(a_pComment);
a_pComment = pComment;
CopyString(a_pComment);
rc = CopyString(a_pComment);
if (rc < 0) return rc;
}
Delete(a_pSection, a_pKey);
iKey = keyval.end();
Expand Down Expand Up @@ -2252,12 +2272,13 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::GetDoubleValue(
return a_nDefault;
}

char * pszSuffix = NULL;
char * pszSuffix = szValue;
double nValue = strtod(szValue, &pszSuffix);

// any invalid strings will return the default value
if (!pszSuffix || *pszSuffix) {
return a_nDefault;
// check if no conversion was performed or if there are trailing characters
if (pszSuffix == szValue || *pszSuffix) {
return a_nDefault;
}

return nValue;
Expand Down Expand Up @@ -2416,7 +2437,7 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::GetSectionSize(
int nCount = 0;
const SI_CHAR * pLastKey = NULL;
typename TKeyVal::const_iterator iKeyVal = section.begin();
for (int n = 0; iKeyVal != section.end(); ++iKeyVal, ++n) {
for (; iKeyVal != section.end(); ++iKeyVal) {
if (!pLastKey || IsLess(pLastKey, iKeyVal->first.pItem)) {
++nCount;
pLastKey = iKeyVal->first.pItem;
Expand Down Expand Up @@ -2448,7 +2469,7 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::GetAllSections(
{
a_names.clear();
typename TSection::const_iterator i = m_data.begin();
for (int n = 0; i != m_data.end(); ++i, ++n ) {
for (; i != m_data.end(); ++i) {
a_names.push_back(i->first);
}
}
Expand All @@ -2474,7 +2495,7 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::GetAllKeys(
const TKeyVal & section = iSection->second;
const SI_CHAR * pLastKey = NULL;
typename TKeyVal::const_iterator iKeyVal = section.begin();
for (int n = 0; iKeyVal != section.end(); ++iKeyVal, ++n ) {
for (; iKeyVal != section.end(); ++iKeyVal) {
if (!pLastKey || IsLess(pLastKey, iKeyVal->first.pItem)) {
a_names.push_back(iKeyVal->first);
pLastKey = iKeyVal->first.pItem;
Expand Down Expand Up @@ -2812,7 +2833,7 @@ CSimpleIniTempl<SI_CHAR,SI_STRLESS,SI_CONVERTER>::DeleteString(
// strings may exist either inside the data block, or they will be
// individually allocated and stored in m_strings. We only physically
// delete those stored in m_strings.
if (a_pString < m_pData || a_pString >= m_pData + m_uDataLen) {
if (!m_pData || a_pString < m_pData || a_pString >= m_pData + m_uDataLen) {
typename TNamesDepend::iterator i = m_strings.begin();
for (;i != m_strings.end(); ++i) {
if (a_pString == i->pItem) {
Expand Down