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

LoadIniSettingsFromDisk: fix out-of-bounds read when file is empty #5351

Closed
wants to merge 1 commit into from

Conversation

quantum5
Copy link
Contributor

When the ini file has zero length, LoadIniSettingsFromDisk passes 0 as ini_size to LoadIniSettingsFromMemory. However, LoadIniSettingsFromMemory understands it as "detect ini size" and promptly runs strlen on the buffer. Here, the buffer is zero-sized, so strlen reads out-of-bounds, trying to find the null terminator in vain.

The solution is to just not call LoadIniSettingsFromMemory when there is nothing to parse anyway.

Example ASan error:

    #0 0x7f6cfc6f4b10 in __interceptor_strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:370
    #1 0x55f13e3fcb18 in ImGui::LoadIniSettingsFromMemory(char const*, unsigned long) /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:11606
    #2 0x55f13e3fca2c in ImGui::LoadIniSettingsFromDisk(char const*) /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:11591
    #3 0x55f13e3fc02f in UpdateSettings /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:11494
    #4 0x55f13e3a14ad in ImGui::NewFrame() /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:4237
    #5 0x55f13e35be4b in igNewFrame (/home/quantum/build/LookingGlass/client/build/looking-glass-client+0x19ae4b)
    #6 0x55f13e2c601b in app_renderOverlay /home/quantum/build/LookingGlass/client/src/app.c:862
    #7 0x55f13e5c01df in egl_render /home/quantum/build/LookingGlass/client/renderers/EGL/egl.c:1099
    #8 0x55f13e29622a in renderThread /home/quantum/build/LookingGlass/client/src/main.c:291
    #9 0x55f13e623261 in threadWrapper /home/quantum/build/LookingGlass/common/src/platform/linux/thread.c:40
    #10 0x7f6cfbe75ea6 in start_thread nptl/pthread_create.c:477
    #11 0x7f6cfba24dee in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfddee)

0x6020000a8631 is located 0 bytes to the right of 1-byte region [0x6020000a8630,0x6020000a8631)
allocated by thread T4 here:
    #0 0x7f6cfc761e8f in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x55f13e37d3c7 in MallocWrapper /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:1019
    #2 0x55f13e399e0e in ImGui::MemAlloc(unsigned long) /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:3603
    #3 0x55f13e3871de in ImFileLoadToMemory(char const*, char const*, unsigned long*, int) /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:1802
    #4 0x55f13e3fc9e8 in ImGui::LoadIniSettingsFromDisk(char const*) /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:11588
    #5 0x55f13e3fc02f in UpdateSettings /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:11494
    #6 0x55f13e3a14ad in ImGui::NewFrame() /home/quantum/build/LookingGlass/repos/cimgui/imgui/imgui.cpp:4237
    #7 0x55f13e35be4b in igNewFrame (/home/quantum/build/LookingGlass/client/build/looking-glass-client+0x19ae4b)
    #8 0x55f13e2c601b in app_renderOverlay /home/quantum/build/LookingGlass/client/src/app.c:862
    #9 0x55f13e5c01df in egl_render /home/quantum/build/LookingGlass/client/renderers/EGL/egl.c:1099
    #10 0x55f13e29622a in renderThread /home/quantum/build/LookingGlass/client/src/main.c:291
    #11 0x55f13e623261 in threadWrapper /home/quantum/build/LookingGlass/common/src/platform/linux/thread.c:40
    #12 0x7f6cfbe75ea6 in start_thread nptl/pthread_create.c:477

When the ini file has zero length, LoadIniSettingsFromDisk passes 0 as
ini_size to LoadIniSettingsFromMemory. However, LoadIniSettingsFromMemory
understands it as "detect ini size" and promptly runs strlen on the
buffer. Here, the buffer is zero-sized, so strlen reads out-of-bounds,
trying to find the null terminator in vain.

The solution is to just not call LoadIniSettingsFromMemory when there
is nothing to parse anyway.
@ocornut
Copy link
Owner

ocornut commented May 29, 2022

Merged, thank you very much !

@ocornut ocornut closed this May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
settings .ini persistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants