Skip to content
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.

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

ERROR: AddressSanitizer: global-buffer-overflow in function ErrorIDToName #923

Open
thientc opened this issue Nov 1, 2022 · 2 comments
Open

Comments

@thientc
Copy link

thientc commented Nov 1, 2022

Instrument Futag found this error with tinyxml2 version 9.0.0 and in current version.

tinyxml2/tinyxml2.cpp

Lines 2501 to 2507 in e45d9d1

/*static*/ const char* XMLDocument::ErrorIDToName(XMLError errorID)
{
TIXMLASSERT( errorID >= 0 && errorID < XML_ERROR_COUNT );
const char* errorName = _errorNames[errorID];
TIXMLASSERT( errorName && errorName[0] );
return errorName;
}

errorID is a variable of XMLError type, which can receive value from XML_SUCCESS (0) to XML_ERROR_COUNT (19)

tinyxml2/tinyxml2.h

Lines 523 to 545 in e45d9d1

enum XMLError {
XML_SUCCESS = 0,
XML_NO_ATTRIBUTE,
XML_WRONG_ATTRIBUTE_TYPE,
XML_ERROR_FILE_NOT_FOUND,
XML_ERROR_FILE_COULD_NOT_BE_OPENED,
XML_ERROR_FILE_READ_ERROR,
XML_ERROR_PARSING_ELEMENT,
XML_ERROR_PARSING_ATTRIBUTE,
XML_ERROR_PARSING_TEXT,
XML_ERROR_PARSING_CDATA,
XML_ERROR_PARSING_COMMENT,
XML_ERROR_PARSING_DECLARATION,
XML_ERROR_PARSING_UNKNOWN,
XML_ERROR_EMPTY_DOCUMENT,
XML_ERROR_MISMATCHED_ELEMENT,
XML_ERROR_PARSING,
XML_CAN_NOT_CONVERT_TEXT,
XML_NO_TEXT_NODE,
XML_ELEMENT_DEPTH_EXCEEDED,
XML_ERROR_COUNT
};

The _errorNames array has 19 elements (from 0 to 18) and was defined here:

tinyxml2/tinyxml2.cpp

Lines 2136 to 2156 in e45d9d1

const char* XMLDocument::_errorNames[XML_ERROR_COUNT] = {
"XML_SUCCESS",
"XML_NO_ATTRIBUTE",
"XML_WRONG_ATTRIBUTE_TYPE",
"XML_ERROR_FILE_NOT_FOUND",
"XML_ERROR_FILE_COULD_NOT_BE_OPENED",
"XML_ERROR_FILE_READ_ERROR",
"XML_ERROR_PARSING_ELEMENT",
"XML_ERROR_PARSING_ATTRIBUTE",
"XML_ERROR_PARSING_TEXT",
"XML_ERROR_PARSING_CDATA",
"XML_ERROR_PARSING_COMMENT",
"XML_ERROR_PARSING_DECLARATION",
"XML_ERROR_PARSING_UNKNOWN",
"XML_ERROR_EMPTY_DOCUMENT",
"XML_ERROR_MISMATCHED_ELEMENT",
"XML_ERROR_PARSING",
"XML_CAN_NOT_CONVERT_TEXT",
"XML_NO_TEXT_NODE",
"XML_ELEMENT_DEPTH_EXCEEDED"
};

So, when errorID gets XML_ERROR_COUNT value, error occurs at instruction const char* errorName = _errorNames[errorID];

The generated fuzzing wrapper is attached below.
ErrorIDToName1.cpp.zip

@pj59
Copy link

pj59 commented Nov 11, 2022

TIXMLASSERT( errorID >= 0 && errorID < XML_ERROR_COUNT );

Looks like a false positive. XML_ERROR_COUNT is a placeholder used as a constant size and not an actual XMLError.

@thientc
Copy link
Author

thientc commented Nov 12, 2022

TIXMLASSERT( errorID >= 0 && errorID < XML_ERROR_COUNT );

Looks like a false positive. XML_ERROR_COUNT is a placeholder used as a constant size and not an actual XMLError.

Is it positive If the TIXMLASSERT does not work on line 95 (commit 1dee28e - version 9.0.0)?

tinyxml2/tinyxml2.h

Lines 82 to 97 in 1dee28e

#if !defined(TIXMLASSERT)
#if defined(TINYXML2_DEBUG)
# if defined(_MSC_VER)
# // "(void)0," is for suppressing C4127 warning in "assert(false)", "assert(true)" and the like
# define TIXMLASSERT( x ) if ( !((void)0,(x))) { __debugbreak(); }
# elif defined (ANDROID_NDK)
# include <android/log.h>
# define TIXMLASSERT( x ) if ( !(x)) { __android_log_assert( "assert", "grinliz", "ASSERT in '%s' at %d.", __FILE__, __LINE__ ); }
# else
# include <assert.h>
# define TIXMLASSERT assert
# endif
#else
# define TIXMLASSERT( x ) {}
#endif
#endif

This line has been fixed with current commit

# define TIXMLASSERT( x ) do {} while(false)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants