Skip to content

Commit

Permalink
Bounds check some tab GetAt()s (microsoft#16016)
Browse files Browse the repository at this point in the history 
`GetAt` can throw if the index is out of range. We don't check that in
some places. This fixes some of those.

I don't think this will take care of microsoft#15689, but it might help?
  • Loading branch information
@zadjii-msft
zadjii-msft authored Oct 5, 2023
1 parent d0c228e commit 5aaddda
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 15 deletions.
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/Resources/en-US/Resources.resw
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,11 @@
</data>
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow" xml:space="preserve">
<value>Active pane moved to "{0}" tab in "{1}" window</value>
<comment>{Locked="{0}"}{Locked="{1}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the tab the pane was moved to. {1} is the name of the window the pane was moved to.</comment>
<comment>{Locked="{0}"}{Locked="{1}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the tab the pane was moved to. {1} is the name of the window the pane was moved to. Replaced in 1.19 by TerminalPage_PaneMovedAnnouncement_ExistingWindow2</comment>
</data>
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow2" xml:space="preserve">
<value>Active pane moved to "{0}" window</value>
<comment>{Locked="{0}"}This text is read out by screen readers upon a successful pane movement. {0} is the name of the window the pane was moved to.</comment>
</data>
<data name="TerminalPage_PaneMovedAnnouncement_NewWindow" xml:space="preserve">
<value>Active pane moved to new window</value>
Expand Down
24 changes: 15 additions & 9 deletions src/cascadia/TerminalApp/TabManagement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ namespace winrt::TerminalApp::implementation
// * B (tabIndex=1): We'll want to focus tab A (now in index 0)
// * C (tabIndex=2): We'll want to focus tab B (now in index 1)
// * D (tabIndex=3): We'll want to focus tab C (now in index 2)
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size());
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size() - 1);
// _UpdatedSelectedTab will do the work of setting up the new tab as
// the focused one, and unfocusing all the others.
auto newSelectedTab{ _tabs.GetAt(newSelectedIndex) };
Expand Down Expand Up @@ -682,7 +682,7 @@ namespace winrt::TerminalApp::implementation
{
uint32_t tabIndexFromControl{};
const auto items{ _tabView.TabItems() };
if (items.IndexOf(tabViewItem, tabIndexFromControl))
if (items.IndexOf(tabViewItem, tabIndexFromControl) && tabIndexFromControl < _tabs.Size())
{
// If IndexOf returns true, we've actually got an index
return _tabs.GetAt(tabIndexFromControl);
Expand Down Expand Up @@ -1039,7 +1039,8 @@ namespace winrt::TerminalApp::implementation
// - suggestedNewTabIndex: the new index of the tab, might get clamped to fit int the tabs row boundaries
// Return Value:
// - <none>
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex)
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex,
const int32_t suggestedNewTabIndex)
{
auto newTabIndex = gsl::narrow_cast<uint32_t>(std::clamp<int32_t>(suggestedNewTabIndex, 0, _tabs.Size() - 1));
if (currentTabIndex != newTabIndex)
Expand Down Expand Up @@ -1081,16 +1082,21 @@ namespace winrt::TerminalApp::implementation

if (from.has_value() && to.has_value() && to != from)
{
auto& tabs{ _tabs };
auto tab = tabs.GetAt(from.value());
tabs.RemoveAt(from.value());
tabs.InsertAt(to.value(), tab);
_UpdateTabIndices();
try
{
auto& tabs{ _tabs };
auto tab = tabs.GetAt(from.value());
tabs.RemoveAt(from.value());
tabs.InsertAt(to.value(), tab);
_UpdateTabIndices();
}
CATCH_LOG();
}

_rearranging = false;

if (to.has_value())
if (to.has_value() &&
*to < gsl::narrow_cast<int32_t>(TabRow().TabView().TabItems().Size()))
{
// Selecting the dropped tab
TabRow().TabView().SelectedIndex(to.value());
Expand Down
7 changes: 2 additions & 5 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2068,9 +2068,6 @@ namespace winrt::TerminalApp::implementation
{
if (const auto pane{ terminalTab->GetActivePane() })
{
// Get the tab title _before_ moving things around in case the tabIdx doesn't point to the right one after the move
const auto tabTitle = _tabs.GetAt(tabIdx).Title();

auto startupActions = pane->BuildStartupActions(0, 1, true, true);
_DetachPaneFromWindow(pane);
_MoveContent(std::move(startupActions.args), windowId, tabIdx);
Expand All @@ -2089,7 +2086,7 @@ namespace winrt::TerminalApp::implementation
{
autoPeer.RaiseNotificationEvent(Automation::Peers::AutomationNotificationKind::ActionCompleted,
Automation::Peers::AutomationNotificationProcessing::ImportantMostRecent,
fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow") }, tabTitle, windowId),
fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow2") }, windowId),
L"TerminalPageMovePaneToExistingWindow" /* unique name for this notification category */);
}
}
Expand All @@ -2106,7 +2103,7 @@ namespace winrt::TerminalApp::implementation

// Moving the pane from the current tab might close it, so get the next
// tab before its index changes.
if (_tabs.Size() > tabIdx)
if (tabIdx < _tabs.Size())
{
auto targetTab = _GetTerminalTabImpl(_tabs.GetAt(tabIdx));
// if the selected tab is not a host of terminals (e.g. settings)
Expand Down

0 comments on commit 5aaddda

Please sign in to comment.