INTEGER_DIVIDE_BY_ZERO c0000094 OpenConsole.exe!Microsoft::Console::VirtualTerminal::AdaptDispatch::_DoLineFeed #20696

32b6220703/src/buffer/out/textBuffer.cpp (L93-L98)

@zadjii-msft commented on GitHub (Oct 19, 2023):

Blame points at L96 specifically:

Presumably, the buffer is empty? dump wasn't clear.

(that's the 1.18 branch. 1.19 re-wrote a lot of this.)

@zadjii-msft commented on GitHub (Oct 19, 2023): Blame points at L96 specifically: https://github.com/microsoft/terminal/blob/32b62207037a18fd7720e29e67df4fe46684ff2c/src/buffer/out/textBuffer.cpp#L93-L98 Presumably, the buffer is empty? dump wasn't clear. (that's the 1.18 branch. 1.19 re-wrote a lot of this.)

claunia commented

@zadjii-msft commented on GitHub (Oct 19, 2023):

Amazingly enough, this definitely is the same failure stack as #15245. Same bucket entirely.

I mean, that makes sense I guess - I fixed something else and assumed that would make this crash impossible, but here we are, in this crash again.

@zadjii-msft commented on GitHub (Oct 19, 2023): Amazingly enough, this definitely is the same failure stack as #15245. Same bucket entirely. I mean, that makes sense I guess - I fixed something else and assumed that would make this crash impossible, but here we are, in this crash again.

claunia commented

@lhecker commented on GitHub (Oct 19, 2023):

An empty buffer would be somewhat surprising: 32b6220703/src/buffer/out/textBuffer.cpp (L42-L44)

@lhecker commented on GitHub (Oct 19, 2023): An empty buffer would be somewhat surprising: https://github.com/microsoft/terminal/blob/32b62207037a18fd7720e29e67df4fe46684ff2c/src/buffer/out/textBuffer.cpp#L42-L44

claunia commented

@lhecker commented on GitHub (Oct 19, 2023):

Oh I see, if it's like the thing that #15298 intended to address, then we're looking at an already deallocated text buffer, due to which the size is 0.

@lhecker commented on GitHub (Oct 19, 2023): Oh I see, if it's like the thing that #15298 intended to address, then we're looking at an already deallocated text buffer, due to which the size is 0.

claunia commented

@zadjii-msft commented on GitHub (Oct 26, 2023):

From the dump in #16204:

_textBuffer in DoWriteConsole is 0x000001fdb0a09220
but by the time we get up in AdaptDispatch::LineFeed, the textBuffer is 0x000001fdb0a41ba0.
- Hmm, no, that can't be right....
In OutputStateMachineEngine::ActionExecute, this->_dispatch->_api->_io->_textBuffer is 0x000001fdb0a09220
Similarly, in AdaptDispatch::LineFeed, this->_api->_io->_textBuffer is 0x000001fdb0a09220...

is there some way that we changed the text buffer between the top of AdaptDispatch::_DoLineFeed (starting L2331) and the textBuffer.GetRowByOffset(newPosition.y).Reset(eraseAttributes); call on L2392? It'd have to be in the _api.SetViewportPosition({ viewport.left, viewport.top + 1 });, right?

oh weird cause AdaptDispatch::_DoLineFeed's textBuffer._size is 120x230. That's a yikes.

@zadjii-msft commented on GitHub (Oct 26, 2023): From the dump in #16204: * `_textBuffer` in `DoWriteConsole` is `0x000001fdb0a09220` * but by the time we get up in `AdaptDispatch::LineFeed`, the `textBuffer` is `0x000001fdb0a41ba0`. * Hmm, no, that can't be right.... * In `OutputStateMachineEngine::ActionExecute`, `this->_dispatch->_api->_io->_textBuffer` is `0x000001fdb0a09220` * Similarly, in `AdaptDispatch::LineFeed`, `this->_api->_io->_textBuffer` is `0x000001fdb0a09220`... is there some way that we changed the text buffer between the top of `AdaptDispatch::_DoLineFeed` (starting L2331) and the `textBuffer.GetRowByOffset(newPosition.y).Reset(eraseAttributes);` call on L2392? It'd have to be in the `_api.SetViewportPosition({ viewport.left, viewport.top + 1 });`, right? oh weird cause `AdaptDispatch::_DoLineFeed`'s `textBuffer._size` is 120x230. **That's a yikes.**

claunia commented

@lhecker commented on GitHub (Oct 26, 2023):

@zadjii-msft I'm not sure if you were there, but in a previous meeting I speculated that it happens when we swap the alt with the main buffer, since that's code I recently touched with the text-reflow PR. Specifically: 74748394c1 (diff-f629ab416d62d00e938348c017c456e3e7b180348a417b875e1a7b37329787f9)
But I don't think that code is incorrect, so it might be another change we recently did.

@lhecker commented on GitHub (Oct 26, 2023): @zadjii-msft I'm not sure if you were there, but in a previous meeting I speculated that it happens when we swap the alt with the main buffer, since that's code I recently touched with the text-reflow PR. Specifically: https://github.com/microsoft/terminal/commit/74748394c17c168843b511dd837268445e5dfd6c#diff-f629ab416d62d00e938348c017c456e3e7b180348a417b875e1a7b37329787f9 But I don't think that code is incorrect, so it might be another change we recently did.

claunia commented

@lone-wolf-akela commented on GitHub (Nov 12, 2023):

Is there anyway to workaround this crash? My users are complaining that my program constantly crash on their machines (due to this bug).

@lone-wolf-akela commented on GitHub (Nov 12, 2023): Is there anyway to workaround this crash? My users are complaining that my program constantly crash on their machines (due to this bug).

claunia commented

@zadjii-msft commented on GitHub (Nov 13, 2023):

@lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them.

However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️

@zadjii-msft commented on GitHub (Nov 13, 2023): @lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them. However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️

claunia commented

@thetestgame commented on GitHub (Nov 14, 2023):

@lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them.

However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️

In our case running it inside Terminal at all caused this. We ended up telling our users to change the default terminal in Windows. Far as reproduction our game already has a program that grants full code access. I could talk to my boss about getting someone on the team access or I could email a built copy of the server which is what currently crashes Terminal.

@thetestgame commented on GitHub (Nov 14, 2023): > @lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them. > > However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️ In our case running it inside Terminal at all caused this. We ended up telling our users to change the default terminal in Windows. Far as reproduction our game already has a program that grants full code access. I could talk to my boss about getting someone on the team access or I could email a built copy of the server which is what currently crashes Terminal.

claunia commented

@lone-wolf-akela commented on GitHub (Nov 14, 2023):

@lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them.

However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️

https://drive.google.com/file/d/17yniBAh4kM5oxVTOTwM-LAUy0RaUpCuL/view?usp=drive_link
Here is our code together with a compiled exe.
Windows terminal will crash at the show9CCN function in patcher.cpp

@lone-wolf-akela commented on GitHub (Nov 14, 2023): > @lone-wolf-akela I suspect if your users start the Terminal first, then run your app from an already running Terminal, this shouldn't affect them. > > However, we haven't ever really gotten a repro for this incarnation of this bug. Could you share your app with us? If you've got a solid repro, it'll make confirming that we've fixed this all that much easier ☺️ https://drive.google.com/file/d/17yniBAh4kM5oxVTOTwM-LAUy0RaUpCuL/view?usp=drive_link Here is our code together with a compiled exe. Windows terminal will crash at the `show9CCN` function in `patcher.cpp`

claunia commented

@dragongrace commented on GitHub (Nov 14, 2023):

Hi Mike, I e-mailed you an example that I hope will make repro easy. Thanks for looking at it!

@dragongrace commented on GitHub (Nov 14, 2023): Hi Mike, I e-mailed you an example that I hope will make repro easy. Thanks for looking at it!

claunia commented

@mkoehlerstb commented on GitHub (Nov 14, 2023):

I can repro this at will with start Terminal with a Powershell tab and run get-childitem. Crash.

@mkoehlerstb commented on GitHub (Nov 14, 2023): I can repro this at will with start Terminal with a Powershell tab and run get-childitem. Crash.

claunia commented

@zadjii-msft commented on GitHub (Nov 17, 2023):

Looks like this is the same thing reported over in #16212 and #16319, which is also seemingly fixed in 1.19. Weird.

There definitely aren't any crash hits for this in 1.19, so that's good at least.

@lhecker The sample repro in 16212 doesn't have any alt buffer hijinks either.

I'm not sure what minimal commit we could use to fix this, however.

(I'd bet this is also secretly #5094 but that's a can of worms to solve)

@zadjii-msft commented on GitHub (Nov 17, 2023): Looks like this is the same thing reported over in #16212 and #16319, which is also seemingly fixed in 1.19. Weird. There definitely aren't any crash hits for this in 1.19, so that's good at least. @lhecker The sample repro in 16212 doesn't have any alt buffer hijinks either. I'm not sure what minimal commit we could use to fix this, however. _(I'd bet this is also secretly #5094 but that's a can of worms to solve)_

claunia commented

@zadjii-msft commented on GitHub (Nov 20, 2023):

notes:

Started from main @ 376737e54.

git bisect bad 32b6220 ->

Bisecting: a merge base must be tested
[63644507dac3ffebff0232d5d0bba30e164096b6] Prevent flickering in nushell due to FTCS marks (#14677)

that was bad

The merge base 63644507dac3ffebff0232d5d0bba30e164096b6 is bad.
This means the bug has been fixed between 63644507dac3ffebff0232d5d0bba30e164096b6 and [376737e54afc301ced37d5416bc836eadbc611d2].

starting again from 376737e54a
63644507da

@zadjii-msft commented on GitHub (Nov 20, 2023): notes: * Started from main @ 376737e54. * `git bisect bad 32b6220` -> ``` Bisecting: a merge base must be tested [63644507dac3ffebff0232d5d0bba30e164096b6] Prevent flickering in nushell due to FTCS marks (#14677) ``` * that was bad * ``` The merge base 63644507dac3ffebff0232d5d0bba30e164096b6 is bad. This means the bug has been fixed between 63644507dac3ffebff0232d5d0bba30e164096b6 and [376737e54afc301ced37d5416bc836eadbc611d2]. ``` * starting again from 376737e54afc301ced37d5416bc836eadbc611d2 * 63644507dac3ffebff0232d5d0bba30e164096b6

claunia commented

@zadjii-msft commented on GitHub (Nov 21, 2023):

literally a day of bisecting later:

612b00cd44 is the first new commit

As I suspected, that doesn't really look like something we can atomically service

@zadjii-msft commented on GitHub (Nov 21, 2023): literally a day of bisecting later: 612b00cd445470fa85d2f71dc94930f03ebe4823 is the first new commit As I suspected, that doesn't really look like something we can atomically service

claunia commented

@DHowett commented on GitHub (Nov 21, 2023):

Well, I guess "it was fixed by rewriting the text buffer" isn't particularly comforting 😆

Maybe there's a quick patch we can throw at it?

@DHowett commented on GitHub (Nov 21, 2023): Well, I guess "it was fixed by rewriting the text buffer" isn't particularly comforting 😆 Maybe there's a quick patch we can throw at it?

claunia commented

@lhecker commented on GitHub (Nov 21, 2023):

const ROW& TextBuffer::GetRowByOffset(const til::CoordType index) const noexcept
{
    // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows.
    const auto offsetIndex = gsl::narrow_cast<size_t>(_firstRow + index) % _storage.size();
    return til::at(_storage, offsetIndex);
}

vs.

ROW& TextBuffer::GetRowByOffset(const til::CoordType index)
{
    // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows.
    auto offset = (_firstRow + index) % _height;

    // Support negative wrap around. This way an index of -1 will
    // wrap to _rowCount-1 and make implementing scrolling easier.
    if (offset < 0)
    {
        offset += _height;
    }

    // We add 1 to the row offset, because row "0" is the one returned by GetScratchpadRow().
    return _getRowByOffsetDirect(gsl::narrow_cast<size_t>(offset) + 1);
}

Could it be that the index parameter is negative? 🤔
(In C/C++ % on negative numbers returns negative numbers.)

@lhecker commented on GitHub (Nov 21, 2023): ```cpp const ROW& TextBuffer::GetRowByOffset(const til::CoordType index) const noexcept { // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows. const auto offsetIndex = gsl::narrow_cast<size_t>(_firstRow + index) % _storage.size(); return til::at(_storage, offsetIndex); } ``` vs. ```cpp ROW& TextBuffer::GetRowByOffset(const til::CoordType index) { // Rows are stored circularly, so the index you ask for is offset by the start position and mod the total of rows. auto offset = (_firstRow + index) % _height; // Support negative wrap around. This way an index of -1 will // wrap to _rowCount-1 and make implementing scrolling easier. if (offset < 0) { offset += _height; } // We add 1 to the row offset, because row "0" is the one returned by GetScratchpadRow(). return _getRowByOffsetDirect(gsl::narrow_cast<size_t>(offset) + 1); } ``` Could it be that the `index` parameter is negative? 🤔 (In C/C++ `%` on negative numbers returns negative numbers.)

claunia commented

@zadjii-msft commented on GitHub (Dec 4, 2023):

You know, you might not be wrong about the idea there's a quick patch we could do here. My theory was always that we were operating on an old buffer, that's already been discarded (hence the weird height=0). But the above commit wouldn't really fix that necessarily, but it does change the way the math gets done. Maybe the discarded buffer thing isn't actually all that breaking, and the new math is resilient to that case.

@zadjii-msft commented on GitHub (Dec 4, 2023): You know, you might not be wrong about the idea there's a quick patch we could do here. My theory was always that we were operating on an old buffer, that's already been discarded (hence the weird height=0). But the above commit wouldn't really fix that necessarily, but it _does_ change the way the math gets done. Maybe the discarded buffer thing isn't actually all that breaking, and the new math is resilient to that case.

claunia commented