Skip to content

Commit

Permalink
fix(console-wallet): fix possible subtract underflow panic in list (#…
Browse files Browse the repository at this point in the history
…5535)

Description
---
Fix possible subtract with underflow panic in console wallet list state

Motivation and Context
---
Ref #5465 

Unable to reproduce this exact panic manually however added a test case
which causes the same panic. I was previously able to reproduce this
every time, either this has been fixed in other changes or the
reproduction steps did not include some mix of circumstances that were
pertinent to the bug. In any case, this PR fixes a bug in select_first
function which uses the binary compliment where a boolean inequality
check was likely intended !self.num_items == 0 vs self.num_items != 0,
though strangely the correct logic is 'if self.num_items == 0 {'. On an
empty list this causes a non-existent first item at index 0 to be
selected, a call to update_list_state would panic.

How Has This Been Tested?
---
Unit test that reproduces the same underflow panic in #5465, although
the send to contact list does not use select_first
Manually

What process can a PR reviewer use to test or verify this change?
---

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
  • Loading branch information
sdbondi authored Jun 28, 2023
1 parent 442d75b commit 8d5e8e6
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ impl BurnTab {

let mut list_state = self
.proofs_list_state
.get_list_state((list_areas[1].height as usize).saturating_sub(3));
.update_list_state((list_areas[1].height as usize).saturating_sub(3));

let window = self.proofs_list_state.get_start_end();
let windowed_view = app_state.get_burnt_proofs_slice(window.0, window.1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl ContactsTab {
self.contacts_list_state.set_num_items(app_state.get_contacts().len());
let mut list_state = self
.contacts_list_state
.get_list_state((list_areas[1].height as usize).saturating_sub(3));
.update_list_state((list_areas[1].height as usize).saturating_sub(3));
let window = self.contacts_list_state.get_start_end();
let windowed_view = app_state.get_contacts_slice(window.0, window.1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl NetworkTab {
self.base_node_list_state.set_num_items(capacity);
let mut base_node_list_state = self
.base_node_list_state
.get_list_state((areas[1].height as usize).saturating_sub(3));
.update_list_state((areas[1].height as usize).saturating_sub(3));

let column_list = MultiColumnList::new()
.highlight_style(Style::default().add_modifier(Modifier::BOLD).fg(Color::Magenta))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ impl SendTab {
}
}

#[allow(dead_code)]
fn draw_contacts<B>(&mut self, f: &mut Frame<B>, area: Rect, app_state: &AppState)
where B: Backend {
let block = Block::default().borders(Borders::ALL).title(Span::styled(
Expand All @@ -223,7 +222,7 @@ impl SendTab {
self.contacts_list_state.set_num_items(app_state.get_contacts().len());
let mut list_state = self
.contacts_list_state
.get_list_state((list_areas[1].height as usize).saturating_sub(3));
.update_list_state((list_areas[1].height as usize).saturating_sub(3));
let window = self.contacts_list_state.get_start_end();
let windowed_view = app_state.get_contacts_slice(window.0, window.1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl TransactionsTab {
self.pending_list_state.set_num_items(app_state.get_pending_txs().len());
let mut pending_list_state = self
.pending_list_state
.get_list_state((area.height as usize).saturating_sub(3));
.update_list_state((area.height as usize).saturating_sub(3));
let window = self.pending_list_state.get_start_end();
let windowed_view = app_state.get_pending_txs_slice(window.0, window.1);

Expand Down Expand Up @@ -186,7 +186,7 @@ impl TransactionsTab {
}
let mut completed_list_state = self
.completed_list_state
.get_list_state((area.height as usize).saturating_sub(3));
.update_list_state((area.height as usize).saturating_sub(3));
let (start, end) = self.completed_list_state.get_start_end();
let windowed_view = &completed_txs[start..end];

Expand Down
59 changes: 45 additions & 14 deletions applications/tari_console_wallet/src/ui/widgets/list_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl WindowedListState {
}
}

pub fn get_list_state(&mut self, height: usize) -> ListState {
pub fn update_list_state(&mut self, height: usize) -> ListState {
// Update the offset based on current offset, selected value and height
self.start = self.offset;
let view_height = height.min(self.num_items);
Expand Down Expand Up @@ -123,7 +123,7 @@ impl WindowedListState {
}

pub fn select_first(&mut self) {
if !self.num_items == 0 {
if self.num_items == 0 {
self.selected = None;
} else {
self.selected = Some(0);
Expand All @@ -136,8 +136,7 @@ impl WindowedListState {

pub fn set_num_items(&mut self, num_items: usize) {
if num_items < self.num_items {
let new_offset = self.offset.saturating_sub(self.num_items - num_items);
self.offset = new_offset;
self.offset = self.offset.saturating_sub(self.num_items - num_items);
}
self.num_items = num_items;
if num_items > 0 {
Expand All @@ -157,6 +156,38 @@ mod test {
use std::convert::TryFrom;

use crate::ui::widgets::WindowedListState;

#[test]
fn test_zero_items() {
let mut list_state = WindowedListState::new();
list_state.previous();
assert_eq!(list_state.selected(), None);
list_state.next();
assert_eq!(list_state.selected(), None);
list_state.update_list_state(5);
assert_eq!(list_state.selected(), None);
assert_eq!(list_state.get_start_end(), (0, 0));

list_state.set_num_items(5);
list_state.set_num_items(0);
list_state.previous();
assert_eq!(list_state.selected(), None);
list_state.next();
assert_eq!(list_state.selected(), None);
list_state.update_list_state(5);
assert_eq!(list_state.selected(), None);
assert_eq!(list_state.get_start_end(), (0, 0));
}

#[test]
fn test_select_first() {
let mut list_state = WindowedListState::new();
list_state.set_num_items(0);
list_state.select_first();
list_state.update_list_state(5);
assert_eq!(list_state.selected(), None);
}

#[test]
fn test_list_offset_update() {
let slist = vec![0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
Expand All @@ -165,24 +196,24 @@ mod test {
let height = 4;
for i in 0..6 {
list_state.next();
let state = list_state.get_list_state(height);
let state = list_state.update_list_state(height);
assert_eq!(state.selected(), Some(i.min(height - 1)));
}
list_state.get_list_state(height);
list_state.update_list_state(height);
let window = list_state.get_start_end();
assert_eq!(slist[window.0..window.1], [2, 3, 4, 5]);

for i in (0..5).rev() {
list_state.previous();
let state = list_state.get_list_state(height);
let state = list_state.update_list_state(height);
assert_eq!(state.selected(), Some(usize::try_from((i - 2i32).max(0)).unwrap()));
}
list_state.get_list_state(height);
list_state.update_list_state(height);
let window = list_state.get_start_end();
assert_eq!(slist[window.0..window.1], [0, 1, 2, 3]);

list_state.previous();
let state = list_state.get_list_state(height);
let state = list_state.update_list_state(height);
assert_eq!(state.selected(), Some(height - 1));
let window = list_state.get_start_end();
assert_eq!(slist[window.0..window.1], [7, 8, 9, 10]);
Expand All @@ -194,12 +225,12 @@ mod test {
list_state.set_num_items(11);
for _ in 0..11 {
list_state.next();
let _state = list_state.get_list_state(4);
let _state = list_state.update_list_state(4);
}

list_state.set_num_items(9);

let _state = list_state.get_list_state(4);
let _state = list_state.update_list_state(4);
let window = list_state.get_start_end();
assert_eq!(window, (5, 9));
}
Expand All @@ -212,13 +243,13 @@ mod test {
// Go to the last item (2 times previous).
list_state.previous();
list_state.previous();
list_state.get_list_state(5);
list_state.update_list_state(5);
assert_eq!(list_state.get_start_end(), (15, 20));
// Resize to 10.
list_state.get_list_state(10);
list_state.update_list_state(10);
assert_eq!(list_state.get_start_end(), (10, 20));
// Resize to 50.
list_state.get_list_state(50);
list_state.update_list_state(50);
assert_eq!(list_state.get_start_end(), (0, 20));
}
}

0 comments on commit 8d5e8e6

Please sign in to comment.