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

widgets/wt_label.c: Skip fillup on negative w->w #8

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

lanodan
Copy link
Contributor

@lanodan lanodan commented Dec 22, 2024

If w->w is negative malloc can fail and return NULL, which results in a segfault.

Also changed it to use calloc instead of malloc to avoid potential overflows.

@Minoru
Copy link
Member

Minoru commented Dec 23, 2024

Hi! It's quite surprising to see Pleroma maintainer fixing bugs in a C library I thought nobody else uses :)

I don't think calloc makes a difference here because the return value is never checked; it could still segfault if widget's width is wide enough. Apart from that, the change makes sense to me and I'm inclined to merge it, but I got two questions:

  1. did you actually trigger the crash? I wonder how w->w can become negative, and I couldn't quickly find it in the code. (But I'm not familiar with STFL's internals, to be honest.)
  2. I see the same malloc invocation in wt_checkbox.c and wt_list.c. Can you please apply the fix there as well?

@lanodan
Copy link
Contributor Author

lanodan commented Dec 24, 2024

Hi! It's quite surprising to see Pleroma maintainer fixing bugs in a C library I thought nobody else uses :)

Heh well I've been using newsboat for many years, like according to package logs on my desktop at least 2018 but could be even earlier.

I don't think calloc makes a difference here because the return value is never checked; it could still segfault if widget's width is wide enough. Apart from that, the change makes sense to me and I'm inclined to merge it, but I got two questions:

Right, could add an assert but given that without it you get null dereference it would crash anyway, meanwhile malloc with an overflow would mean wrong size being allocated which is much harder to diagnose.

  1. did you actually trigger the crash? I wonder how w->w can become negative, and I couldn't quickly find it in the code. (But I'm not familiar with STFL's internals, to be honest.)

Yeah, newsboat (2.13, I have rust masked out but well that's on me) was crashing on me when the terminal would get resized.

Here's backtrace and variables at time of crash: lldb-newsboat.log

  1. I see the same malloc invocation in wt_checkbox.c and wt_list.c. Can you please apply the fix there as well?

Yeah, makes sense.

If w->w is negative malloc can fail and return NULL, which results in a segfault.

Also changed it to use calloc instead of malloc to avoid potential overflows.
@Minoru
Copy link
Member

Minoru commented Dec 24, 2024

Got it; thanks!

@Minoru Minoru merged commit bbb2404 into newsboat:master Dec 24, 2024
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.

2 participants