Skip to content

Commit 32b6220

Browse files
zadjii-msftDHowett
authored andcommitted
Bounds check some tab GetAt()s (#16016)
`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 #15689, but it might help? (cherry picked from commit 5aaddda) Service-Card-Id: 90731980 Service-Version: 1.18
1 parent daf03ba commit 32b6220

File tree

3 files changed

+22
-15
lines changed

3 files changed

+22
-15
lines changed

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+5-1
Original file line numberDiff line numberDiff line change
@@ -882,7 +882,11 @@
882882
</data>
883883
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow" xml:space="preserve">
884884
<value>Active pane moved to "{0}" tab in "{1}" window</value>
885-
<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>
885+
<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>
886+
</data>
887+
<data name="TerminalPage_PaneMovedAnnouncement_ExistingWindow2" xml:space="preserve">
888+
<value>Active pane moved to "{0}" window</value>
889+
<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>
886890
</data>
887891
<data name="TerminalPage_PaneMovedAnnouncement_NewWindow" xml:space="preserve">
888892
<value>Active pane moved to new window</value>

src/cascadia/TerminalApp/TabManagement.cpp

+15-9
Original file line numberDiff line numberDiff line change
@@ -498,7 +498,7 @@ namespace winrt::TerminalApp::implementation
498498
// * B (tabIndex=1): We'll want to focus tab A (now in index 0)
499499
// * C (tabIndex=2): We'll want to focus tab B (now in index 1)
500500
// * D (tabIndex=3): We'll want to focus tab C (now in index 2)
501-
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size());
501+
const auto newSelectedIndex = std::clamp<int32_t>(tabIndex - 1, 0, _tabs.Size() - 1);
502502
// _UpdatedSelectedTab will do the work of setting up the new tab as
503503
// the focused one, and unfocusing all the others.
504504
auto newSelectedTab{ _tabs.GetAt(newSelectedIndex) };
@@ -665,7 +665,7 @@ namespace winrt::TerminalApp::implementation
665665
{
666666
uint32_t tabIndexFromControl{};
667667
const auto items{ _tabView.TabItems() };
668-
if (items.IndexOf(tabViewItem, tabIndexFromControl))
668+
if (items.IndexOf(tabViewItem, tabIndexFromControl) && tabIndexFromControl < _tabs.Size())
669669
{
670670
// If IndexOf returns true, we've actually got an index
671671
return _tabs.GetAt(tabIndexFromControl);
@@ -1022,7 +1022,8 @@ namespace winrt::TerminalApp::implementation
10221022
// - suggestedNewTabIndex: the new index of the tab, might get clamped to fit int the tabs row boundaries
10231023
// Return Value:
10241024
// - <none>
1025-
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex)
1025+
void TerminalPage::_TryMoveTab(const uint32_t currentTabIndex,
1026+
const int32_t suggestedNewTabIndex)
10261027
{
10271028
auto newTabIndex = gsl::narrow_cast<uint32_t>(std::clamp<int32_t>(suggestedNewTabIndex, 0, _tabs.Size() - 1));
10281029
if (currentTabIndex != newTabIndex)
@@ -1064,16 +1065,21 @@ namespace winrt::TerminalApp::implementation
10641065

10651066
if (from.has_value() && to.has_value() && to != from)
10661067
{
1067-
auto& tabs{ _tabs };
1068-
auto tab = tabs.GetAt(from.value());
1069-
tabs.RemoveAt(from.value());
1070-
tabs.InsertAt(to.value(), tab);
1071-
_UpdateTabIndices();
1068+
try
1069+
{
1070+
auto& tabs{ _tabs };
1071+
auto tab = tabs.GetAt(from.value());
1072+
tabs.RemoveAt(from.value());
1073+
tabs.InsertAt(to.value(), tab);
1074+
_UpdateTabIndices();
1075+
}
1076+
CATCH_LOG();
10721077
}
10731078

10741079
_rearranging = false;
10751080

1076-
if (to.has_value())
1081+
if (to.has_value() &&
1082+
*to < gsl::narrow_cast<int32_t>(TabRow().TabView().TabItems().Size()))
10771083
{
10781084
// Selecting the dropped tab
10791085
TabRow().TabView().SelectedIndex(to.value());

src/cascadia/TerminalApp/TerminalPage.cpp

+2-5
Original file line numberDiff line numberDiff line change
@@ -2026,9 +2026,6 @@ namespace winrt::TerminalApp::implementation
20262026
{
20272027
if (const auto pane{ terminalTab->GetActivePane() })
20282028
{
2029-
// Get the tab title _before_ moving things around in case the tabIdx doesn't point to the right one after the move
2030-
const auto tabTitle = _tabs.GetAt(tabIdx).Title();
2031-
20322029
auto startupActions = pane->BuildStartupActions(0, 1, true, true);
20332030
_DetachPaneFromWindow(pane);
20342031
_MoveContent(std::move(startupActions.args), windowId, tabIdx);
@@ -2047,7 +2044,7 @@ namespace winrt::TerminalApp::implementation
20472044
{
20482045
autoPeer.RaiseNotificationEvent(Automation::Peers::AutomationNotificationKind::ActionCompleted,
20492046
Automation::Peers::AutomationNotificationProcessing::ImportantMostRecent,
2050-
fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow") }, tabTitle, windowId),
2047+
fmt::format(std::wstring_view{ RS_(L"TerminalPage_PaneMovedAnnouncement_ExistingWindow2") }, windowId),
20512048
L"TerminalPageMovePaneToExistingWindow" /* unique name for this notification category */);
20522049
}
20532050
}
@@ -2064,7 +2061,7 @@ namespace winrt::TerminalApp::implementation
20642061

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

0 commit comments

Comments
 (0)