Skip to content

Commit

Permalink
stab in the dark to fix x86 tests. (#4202)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request
Perform checking on `std::basic_string_view<T>.substr()` calls to
prevent running out of bounds and sporadic Privileged Instruction throws
during x86 tests.

## PR Checklist
* [x] Closes the x86 tests failing all over the place since #4125 for no
  apparent reason
* [x] I work here
* [x] Tests pass

## Detailed Description of the Pull Request / Additional comments
It appears that not all `std::basic_string_view<T>.substr()` calls are
created equally. I rooted around for other versions of the code in our
source tree and found several versions that were less careful about
checking the start position and the size than the one that appears when
building locally on dev machines.

My theory is that one of these older versions is deployed somewhere in
the CI. Instead of clamping down the size parameter appropriately or
throwing correctly when the position is out of bounds, I believe that
it's just creating a substring with a bad range over an
invalid/uninitialized memory region. Then when the test operates on
that, sometimes it turns out to trigger the privileged instruction
NTSTATUS error we are seeing in CI.

## Test Procedure
1. Fixed the thing
2. Ran the CI and it worked
3. Reverted everything and turned off all of the CI build except just
   the parser tests (and supporting libraries)
4. Ran CI and it failed
5. Put the fix back on top (cherry-pick)
6. It worked.
7. Ran it again.
8. It worked.
9. Turn all the rest of the CI build back on

(cherry picked from commit 4129ceb)
  • Loading branch information
miniksa authored and DHowett committed Jan 24, 2020
1 parent 5ecff02 commit 0c77366
Showing 1 changed file with 11 additions and 9 deletions.
20 changes: 11 additions & 9 deletions src/terminal/parser/stateMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1269,13 +1269,17 @@ void StateMachine::ProcessString(const std::wstring_view string)
{
if (_isActionableFromGround(string.at(current))) // If the current char is the start of an escape sequence, or should be executed in ground state...
{
// Because the _run above is composed INCLUDING current, we must
// trim it off here since we just determined it's actionable
// and only pass through everything before it.
const auto allLeadingUpTo = _run.substr(0, _run.size() - 1);
if (!_run.empty())
{
// Because the _run above is composed INCLUDING current, we must
// trim it off here since we just determined it's actionable
// and only pass through everything before it.
const auto allLeadingUpTo = _run.substr(0, _run.size() - 1);

_engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run...
_trace.DispatchPrintRunTrace(allLeadingUpTo);
}

_engine->ActionPrintString(allLeadingUpTo); // ... print all the chars leading up to it as part of the run...
_trace.DispatchPrintRunTrace(allLeadingUpTo);
_processingIndividually = true; // begin processing future characters individually...
start = current;
continue;
Expand All @@ -1290,9 +1294,7 @@ void StateMachine::ProcessString(const std::wstring_view string)
// When we leave the loop, current has been advanced to the length of the string itself
// (or one past the array index to the final char) so this `substr` operation doesn't +1
// to include the final character (unlike the one inside the top of the loop above.)
// NOTE: std::basic_string_view will auto-trim excessively large sizes down to the valid length
// so passing something too large in the second parameter WILL NOT FAIL.
_run = string.substr(start, current - start);
_run = start < string.size() ? string.substr(start) : std::wstring_view{};

// If we're at the end of the string and have remaining un-printed characters,
if (!_processingIndividually && !_run.empty())
Expand Down

0 comments on commit 0c77366

Please sign in to comment.