-
Notifications
You must be signed in to change notification settings - Fork 29
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
Fix FormatAllocateString complexity #808
base: main
Are you sure you want to change the base?
Conversation
{ | ||
FREE_MEMORY(stringToReturn); | ||
return NULL; | ||
} | ||
return stringToReturn; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great idea! Just a nit suggestion, same code, just bit changed to only return once after initial input args validation (for consistence with rest of code), how about this?
char* FormatAllocateString(const char* format, ...)
{
char* stringToReturn = NULL;
int formatResult = 0;
int sizeOfBuffer = 0;
if (NULL == format)
{
return stringToReturn;
}
va_list arguments;
va_start(arguments, format);
// snprintf returns the required buffer size, excluding the null terminator
sizeOfBuffer = vsnprintf(NULL, 0, format, arguments);
va_end(arguments);
if (sizeOfBuffer > 0)
{
if (NULL != (stringToReturn = malloc((size_t)sizeOfBuffer + 1)))
{
va_start(arguments, format);
formatResult = vsnprintf(stringToReturn, sizeOfBuffer + 1, format, arguments);
va_end(arguments);
if ((formatResult < 0) || (formatResult > sizeOfBuffer))
{
FREE_MEMORY(stringToReturn);
}
}
}
return stringToReturn;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
FormatAllocateString has multiple issues:
malloc,snprintf,free
loop until it finds the right size. This has an O(n^2) complexity and is slow for longer strings.DuplicateString
at the endRewrite FormatAllocateString to call
snprintf
once to calculate the required buffer size, thenmalloc
the right buffer and then format the string.This change is motivated by hitting this edge case through fuzzing
CheckFileSystemMountingOption
where a lot of reason strings get added to the reason buffer. I've added a test case for long strings and a fuzzer case whose runtime goes from ~3s to 10ms.Checklist
main
branch prior to this PR submission.main
branch.