Skip to content
New issue

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

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

Already on GitHub? # to your account

ImFormatStringToTempBufferV crashes if a null is passed for a %s parameter #7016

Closed
codefrog2002 opened this issue Nov 15, 2023 · 1 comment
Labels

Comments

@codefrog2002
Copy link
Contributor

codefrog2002 commented Nov 15, 2023

Version: 1.90
Branch: master

Many printf implementations will accept a nullptr when passed to a %s format.
That is, printf("%s", nullptr); will emit "(null)" on many implementations of printf.
I recognize this is undefined behavior, but it's easy to handle it gracefully.

ImFormatStringToTempBufferV instead crashes during its attempt to optimize around the heavy ImFormatStringV.

See this block of code - ImGui.cpp, void ImFormatStringToTempBufferV

    if (fmt[0] == '%' && fmt[1] == 's' && fmt[2] == 0)
    {
        const char* buf = va_arg(args, const char*); // Skip formatting when using "%s"
        
        if (buf == nullptr) buf = "(null)";  // << I added this to prevent crash from calling strlen(nullptr)
        
        *out_buf = buf;
        if (out_buf_end) { *out_buf_end = buf + strlen(buf); }
    }

And as of the newest version (1.90) there's a subsequent code block (looking for "%.*s") with the same issue.

    else if (fmt[0] == '%' && fmt[1] == '.' && fmt[2] == '*' && fmt[3] == 's' && fmt[4] == 0)
    {
        int buf_len = va_arg(args, int); // Skip formatting when using "%.*s"
        const char* buf = va_arg(args, const char*);
        if (buf == nullptr) buf = "(null)"; // Added  to prevent nullptr + length 
        *out_buf = buf;
        *out_buf_end = buf + buf_len; // Disallow not passing 'out_buf_end' here. User is expected to use it.
    }
@ocornut ocornut added the bug label Nov 15, 2023
ocornut added a commit that referenced this issue Nov 15, 2023
@ocornut
Copy link
Owner

ocornut commented Nov 15, 2023

I have pushed your proposed change as 7bb0a52, thanks!

Note that the %..*s code path needs to account for clamping buf_len (I quickly tested msvc sprintf behavior and it did the same).

@ocornut ocornut closed this as completed Nov 15, 2023
ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Nov 22, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants