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 null deref with y_offset in nk_group and nk_listview #584

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mtijanic
Copy link

@mtijanic mtijanic commented Oct 30, 2023

We hit a rare null deref on y_offset in nk_group_scrolled_offset_begin(), that I think happens like this:

    // snippet from nk_group_begin_titled()

    x_offset = nk_find_value(win, id_hash);
    if (!x_offset) {
        x_offset = nk_add_value(ctx, win, id_hash, 0);
        y_offset = nk_add_value(ctx, win, id_hash+1, 0);
        NK_ASSERT(x_offset);
        NK_ASSERT(y_offset);
        if (!x_offset || !y_offset) return 0;
        *x_offset = *y_offset = 0;
    } else y_offset = nk_find_value(win, id_hash+1);
    return nk_group_scrolled_offset_begin(ctx, x_offset, y_offset, title, flags);

First, we don't find the x_offset, so we go into the if() branch. There, we manage to add x_offset but not y_offset. This causes it to bail early. Then, next frame, it will find x_offset and go into the else branch. There, it fails to find y_offset, and eventually calls into nk_group_scrolled_offset_begin() with y_offset = NULL.

Never got a local repro so can't say for sure if the existing NK_ASSERT(y_offset) was firing, but end user reports that this patch fixes it.

I think this is one of the issues that was reported in #513

Comment on lines +22830 to +22838
} else {
y_offset = nk_find_value(win, id_hash+1);
if (!y_offset) {
y_offset = nk_add_value(ctx, win, id_hash+1, 0);
NK_ASSERT(y_offset);
if (!y_offset) return 0;
*y_offset = 0;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find, thanks a lot. Two things...

  1. Make sure to update the change in the src directory too. Otherwise the next time the paq.sh file is run, we'll lose this change.
  2. Also, what do you think about updating this pattern in both nuklear_group.c and nuklear_list_view.c too? Could probably run into the same issue there, as it's using a similar pattern.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Ah, right, sorry! I just ported the change from the vendored file in my project, I didn't read up on how development actually goes here. Sorry. Do I need to also edit nuklear.h or will that be handled automatically when you pack it up?
  2. Yep, seems similar. Do you want the same change there as well, or do you have some nicer way to generalize it in mind?

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. No need to appologize. Can be confusing. You don't absolutely need to edit nuklear.h. Anyone can run the script and it'll get updated eventually. Whichever is easier for you
  2. Same way is fine 🤷

Copy link
Author

Choose a reason for hiding this comment

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

Done. I left it as smaller commits, but let me know if you prefer a squash. Or just squash when merging.

@mtijanic mtijanic changed the title Fix null deref in nk_group_scrolled_offset_begin Fix null deref with y_offset in nk_group and nk_listview Nov 3, 2023
@mtijanic mtijanic closed this Nov 3, 2023
@mtijanic mtijanic deleted the patch-1 branch November 3, 2023 15:39
@mtijanic mtijanic restored the patch-1 branch November 3, 2023 15:39
@mtijanic mtijanic reopened this Nov 3, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants