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

Fix buffer overflow on X11 error bug #1748

Merged
merged 1 commit into from
Feb 25, 2024
Merged

Fix buffer overflow on X11 error bug #1748

merged 1 commit into from
Feb 25, 2024

Conversation

Caellian
Copy link
Collaborator

Closes #1739.

Additional changes

  • Skip descendant checking if cursor over desktop in query_x11_window_at_pos.
  • Simplify check for whether cursor is over conky.

Skip descendant checking if cursor over desktop in query_x11_window_at_pos.
Simplify and correct check for whether cursor is over conky.

Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
@github-actions github-actions bot added the sources PR modifies project sources label Feb 25, 2024
Copy link

netlify bot commented Feb 25, 2024

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit a3db446
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/65da9fb76cec2a000861d45f

@@ -258,7 +258,7 @@ static int x11_error_handler(Display *d, XErrorEvent *err) {
const char *minor = xcb_errors_get_name_for_minor_code(
xcb_errors_ctx, err->request_code, err->minor_code);
if (minor != nullptr) {
const std::size_t size = strlen(base_name) + strlen(extension) + 4;
const std::size_t size = strlen(major) + strlen(minor) + 4;
Copy link
Collaborator Author

@Caellian Caellian Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After stepping through the code with gdb, this is what was causing the crash. Not sure why it's snprintf fails over code_description being too small (isn't that the point of snprintf - to truncate?).

@Caellian
Copy link
Collaborator Author

Caellian commented Feb 25, 2024

Another call to snprintf on line 288 crashed in the linked issue (in the gdb.txt file posted by starvald):

#8  0x00005555555d8a68 in snprintf (__fmt=0x555555602168 "error code: [major: %i, minor: %i]", __n=37, __s=0x55555588b620 "%", <incomplete sequence \337>, __s=<optimized out>, __n=<optimized out>, __fmt=<optimized out>) at /usr/include/bits/stdio2.h:54

The one I already fixed in this PR was the one I could figure out quickly (and prevent a crash locally). The second one (called because user didn't have XDB errors installed), I can't force to cause a crash:

  • I intentionally set the size to 20, the string gets truncated (unlike the one I modified which crashed here).
    • The same assertion causes the crash in both cases.
  • I replaced return last_descendant(display, last); with return last_descendant(display, 0); this only causes the error to get consistently reported, but doesn't crash conky.

After digging deeper into implementation of snprintf and staring at the gdb output, it looks like code_description is initialized to length of 1, even though size is 37:

if (code_description == nullptr) {
    const std::size_t size = 37;
    code_description = new char[size];
    snprintf(code_description, size, "error code: [major: %i, minor: %i]",
             err->request_code, err->minor_code);
    code_allocated = true;
  }

because slen in the following call:

#7  0x00007ffff7133945 in ___snprintf_chk (s=s@entry=0x55555588b620 "%", <incomplete sequence \337>, maxlen=maxlen@entry=37, flag=flag@entry=1, slen=slen@entry=1, format=format@entry=0x555555602168 "error code: [major: %i, minor: %i]") at snprintf_chk.c:29

is "target buffer length", and it's equal to 1, while maxlen is 37.
That makes no sense as both come from the same constant.

Which leaves the following options:

  • new char[size] creates a buffer of length 1, which is unlikely.
  • user has buggy snprintf implementation.
  • I'm missing something.

@brndnmtthws brndnmtthws added the bug Bug report or bug fix PR label Feb 25, 2024
@brndnmtthws brndnmtthws merged commit 27d64fe into brndnmtthws:main Feb 25, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report or bug fix PR sources PR modifies project sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: conky 1.19.7 crashing when mouse moves on opensuse Tumbleweed
2 participants