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.

Sign up for GitHub

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

Reduce likelihood of races between stdout and cooked stdin reads #18326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce likelihood of races between stdout and cooked stdin reads #18326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 38 additions & 15 deletions src/host/readDataCooked.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
THROW_IF_FAILED(_screenInfo.GetMainBuffer().AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle));
#endif

const auto cursorPos = _getViewportCursorPosition();
_originInViewport = cursorPos;

if (!initialData.empty())
{
// The console API around `nInitialChars` in `CONSOLE_READCONSOLE_CONTROL` is pretty weird.
Expand Down Expand Up @@ -94,6 +91,7 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
// It replicates part of the _redisplay() logic to layout the text at various
// starting positions until it finds one that matches the current cursor position.

const auto cursorPos = _getViewportCursorPosition();
const auto size = _screenInfo.GetVtPageArea().Dimensions();

// Guess the initial cursor position based on the string length, assuming that 1 char = 1 column.
Expand Down Expand Up @@ -285,22 +283,29 @@ bool COOKED_READ_DATA::Read(const bool isUnicode, size_t& numBytes, ULONG& contr

// Printing wide glyphs at the end of a row results in a forced line wrap and a padding whitespace to be inserted.
// When the text buffer resizes these padding spaces may vanish and the _distanceCursor and _distanceEnd measurements become inaccurate.
// To fix this, this function is called before a resize and will clear the input line. Afterwards, RedrawAfterResize() will restore it.
// To fix this, this function is called before a resize and will clear the input line. Afterward, RedrawAfterResize() will restore it.
void COOKED_READ_DATA::EraseBeforeResize()
{
// If we've already erased the buffer, we don't need to do it again.
if (_redrawPending)
{
return;
}

// If we don't have an origin, we've never had user input, and consequently there's nothing to erase.
if (!_originInViewport)
{
return;
}

_redrawPending = true;

// Position the cursor the start of the prompt before reflow.
// Then, after reflow, we'll be able to ask the buffer where it went (the new origin).
// This uses the buffer APIs directly, so that we don't emit unnecessary VT into ConPTY's output.
auto& textBuffer = _screenInfo.GetTextBuffer();
auto& cursor = textBuffer.GetCursor();
auto cursorPos = _originInViewport;
auto cursorPos = *_originInViewport;
_screenInfo.GetVtPageArea().ConvertFromOrigin(&cursorPos);
cursor.SetPosition(cursorPos);
}
Expand All @@ -316,7 +321,10 @@ void COOKED_READ_DATA::RedrawAfterResize()
_redrawPending = false;

// Get the new cursor position after the reflow, since it may have changed.
_originInViewport = _getViewportCursorPosition();
if (_originInViewport)
{
_originInViewport = _getViewportCursorPosition();
}

// Ensure that we don't use any scroll sequences or try to clear previous pager contents.
// They have all been erased with the CSI J above.
Expand Down Expand Up @@ -348,7 +356,7 @@ bool COOKED_READ_DATA::PresentingPopup() const noexcept
return !_popups.empty();
}

til::point_span COOKED_READ_DATA::GetBoundaries() const noexcept
til::point_span COOKED_READ_DATA::GetBoundaries() noexcept
{
const auto viewport = _screenInfo.GetViewport();
const auto virtualViewport = _screenInfo.GetVtPageArea();
Expand All @@ -357,7 +365,7 @@ til::point_span COOKED_READ_DATA::GetBoundaries() const noexcept
const til::point max{ viewport.RightInclusive(), viewport.BottomInclusive() };

// Convert from VT-viewport-relative coordinate space back to the console one.
auto beg = _originInViewport;
auto beg = _getOriginInViewport();
virtualViewport.ConvertFromOrigin(&beg);

// Since the pager may be longer than the viewport is tall, we need to clamp the coordinates to still remain within
Expand Down Expand Up @@ -831,6 +839,20 @@ til::point COOKED_READ_DATA::_getViewportCursorPosition() const noexcept
return cursorPos;
}

// Some applications initiate a read on stdin and _then_ print the prompt prefix to stdout.
// While that's not correct (because it's a race condition), we can make it significantly
// less bad by delaying the calculation of the origin until we actually need it.
// This turns it from a race between application and terminal into a race between
// application and user, which is much less likely to hit.
til::point COOKED_READ_DATA::_getOriginInViewport() noexcept
{
if (!_originInViewport)
{
_originInViewport.emplace(_getViewportCursorPosition());
}
return *_originInViewport;
}

void COOKED_READ_DATA::_replace(size_t offset, size_t remove, const wchar_t* input, size_t count)
{
const auto size = _buffer.size();
Expand Down Expand Up @@ -885,7 +907,8 @@ void COOKED_READ_DATA::_redisplay()
}

const auto size = _screenInfo.GetVtPageArea().Dimensions();
auto originInViewportFinal = _originInViewport;
auto originInViewport = _getOriginInViewport();
auto originInViewportFinal = originInViewport;
til::point cursorPositionFinal;
til::point pagerPromptEnd;
std::vector<Line> lines;
Expand All @@ -894,7 +917,7 @@ void COOKED_READ_DATA::_redisplay()
// and if MSVC says that then that must be true.
for (;;)
{
cursorPositionFinal = { _originInViewport.x, 0 };
cursorPositionFinal = { originInViewport.x, 0 };

// Construct the first line manually so that it starts at the correct horizontal position.
LayoutResult res{ .column = cursorPositionFinal.x };
Expand Down Expand Up @@ -1057,8 +1080,8 @@ void COOKED_READ_DATA::_redisplay()
if (gsl::narrow_cast<til::CoordType>(lines.size()) > size.height && originInViewportFinal.x != 0)
{
lines.clear();
_originInViewport.x = 0;
_bufferDirtyBeg = 0;
originInViewport.x = 0;
originInViewportFinal = {};
continue;
}
Expand Down Expand Up @@ -1092,7 +1115,7 @@ void COOKED_READ_DATA::_redisplay()
if (_clearPending)
{
_clearPending = false;
_appendCUP(output, _originInViewport);
_appendCUP(output, originInViewport);
output.append(L"\x1b[J");
}

Expand All @@ -1111,7 +1134,7 @@ void COOKED_READ_DATA::_redisplay()
// The check for origin == {0,0} is important because it ensures that we "own" the entire viewport and
// that scrolling our contents doesn't scroll away the user's output that may still be in the viewport.
// (Anything below the origin is assumed to belong to us.)
if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && _originInViewport == til::point{})
if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && originInViewport == til::point{})
{
const auto deltaAbs = abs(delta);
til::CoordType beg = 0;
Expand Down Expand Up @@ -1179,7 +1202,7 @@ void COOKED_READ_DATA::_redisplay()

for (til::CoordType i = 0; i < pagerHeight; i++)
{
const auto row = std::min(_originInViewport.y + i, size.height - 1);
const auto row = std::min(originInViewport.y + i, size.height - 1);

// If the last write left the cursor at the end of a line, the next write will start at the beginning of the next line.
// This avoids needless calls to _appendCUP. The reason it's here and not at the end of the loop is similar to how
Expand Down Expand Up @@ -1223,7 +1246,7 @@ void COOKED_READ_DATA::_redisplay()

if (pagerHeight < pagerHeightPrevious)
{
const auto row = std::min(_originInViewport.y + pagerHeight, size.height - 1);
const auto row = std::min(originInViewport.y + pagerHeight, size.height - 1);
_appendCUP(output, { 0, row });
output.append(L"\x1b[K");

Expand Down
5 changes: 3 additions & 2 deletions src/host/readDataCooked.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class COOKED_READ_DATA final : public ReadData
void SetInsertMode(bool insertMode) noexcept;
bool IsEmpty() const noexcept;
bool PresentingPopup() const noexcept;
til::point_span GetBoundaries() const noexcept;
til::point_span GetBoundaries() noexcept;

private:
static constexpr size_t CommandNumberMaxInputLength = 5;
Expand Down Expand Up @@ -129,6 +129,7 @@ class COOKED_READ_DATA final : public ReadData
void _handlePostCharInputLoop(bool isUnicode, size_t& numBytes, ULONG& controlKeyState);
void _transitionState(State state) noexcept;
til::point _getViewportCursorPosition() const noexcept;
til::point _getOriginInViewport() noexcept;
void _replace(size_t offset, size_t remove, const wchar_t* input, size_t count);
void _replace(const std::wstring_view& str);
std::wstring_view _slice(size_t from, size_t to) const noexcept;
Expand Down Expand Up @@ -166,7 +167,7 @@ class COOKED_READ_DATA final : public ReadData
bool _redrawPending = false;
bool _clearPending = false;

til::point _originInViewport;
std::optional<til::point> _originInViewport;
// This value is in the pager coordinate space. (0,0) is the first character of the
// first line, independent on where the prompt actually appears on the screen.
// The coordinate is "end exclusive", so the last character is 1 in front of it.
Expand Down
2 changes: 1 addition & 1 deletion src/host/selectionInput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ bool Selection::_HandleMarkModeSelectionNav(const INPUT_KEY_INFO* const pInputKe
// - If true, the boundaries returned are valid. If false, they should be discarded.
[[nodiscard]] bool Selection::s_GetInputLineBoundaries(_Out_opt_ til::point* const pcoordInputStart, _Out_opt_ til::point* const pcoordInputEnd)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

if (gci.HasPendingCookedRead())
{
Expand Down
Loading