Skip to content

fix(palette): fix u16 overflow in luma() causing grayscale theme crash#1832

Closed
wdw8276 wants to merge 2 commits into
Hmbown:mainfrom
wdw8276:fix/luma-u16-overflow
Closed

fix(palette): fix u16 overflow in luma() causing grayscale theme crash#1832
wdw8276 wants to merge 2 commits into
Hmbown:mainfrom
wdw8276:fix/luma-u16-overflow

Conversation

@wdw8276
Copy link
Copy Markdown
Contributor

@wdw8276 wdw8276 commented May 20, 2026

Summary

  • luma() in palette.rs uses u16 for intermediate arithmetic
  • u16::from(r) * 299 overflows u16::MAX (65535) when r >= 220
  • u16::from(g) * 587 overflows when g >= 112
  • In debug builds, this causes a panic (attempt to add with overflow) whenever the grayscale theme processes a bright RGB color
  • Fix: use u32 for intermediate calculations (result still fits in u8)

Reproduction

Switch to the Grayscale theme (/theme → select option 4) in a debug build — TUI immediately crashes with a panic logged to ~/.deepseek/crashes/.

Fix

// Before
fn luma(r: u8, g: u8, b: u8) -> u8 {
    (((u16::from(r) * 299) + (u16::from(g) * 587) + (u16::from(b) * 114)) / 1000) as u8
}

// After
fn luma(r: u8, g: u8, b: u8) -> u8 {
    (((u32::from(r) * 299) + (u32::from(g) * 587) + (u32::from(b) * 114)) / 1000) as u8
}

u16::from(r) * 299 overflows u16::MAX (65535) when r >= 220,
and u16::from(g) * 587 overflows when g >= 112, causing a panic
in debug builds whenever the grayscale theme processes bright colors.

Use u32 for intermediate arithmetic to prevent overflow.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the luma function in crates/tui/src/palette.rs to use u32 for intermediate calculations, effectively preventing integer overflows. The reviewer suggested improving the accuracy of the luminance mapping by adding a rounding offset before division and recommended simplifying the expression by removing redundant parentheses.

Comment thread crates/tui/src/palette.rs Outdated

fn luma(r: u8, g: u8, b: u8) -> u8 {
(((u16::from(r) * 299) + (u16::from(g) * 587) + (u16::from(b) * 114)) / 1000) as u8
(((u32::from(r) * 299) + (u32::from(g) * 587) + (u32::from(b) * 114)) / 1000) as u8
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

While the switch to u32 correctly prevents the overflow and subsequent panic, the current implementation uses integer division which truncates the result. For more accurate luminance mapping to the grayscale palette, consider adding a rounding offset (500) before dividing by 1000. Additionally, the inner parentheses around the multiplications are redundant and can be removed to improve readability.

Suggested change
(((u32::from(r) * 299) + (u32::from(g) * 587) + (u32::from(b) * 114)) / 1000) as u8
((u32::from(r) * 299 + u32::from(g) * 587 + u32::from(b) * 114 + 500) / 1000) as u8

Apply Gemini review suggestions:
- Add +500 before /1000 for proper integer rounding
- Remove redundant inner parentheses
@wdw8276
Copy link
Copy Markdown
Contributor Author

wdw8276 commented May 20, 2026

Thanks for the review! Both suggestions have been applied in the latest commit:

  • Added +500 before /1000 for proper integer rounding
  • Removed the redundant inner parentheses

@Hmbown
Copy link
Copy Markdown
Owner

Hmbown commented May 21, 2026

Thanks for the grayscale overflow patch. This was harvested into v0.8.40 release PR #1823 as commit ae1bb9d, and #1823 is now CI-green. Closing as superseded by the release branch; thank you for catching the crash path.

@Hmbown Hmbown closed this May 21, 2026
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